Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to the bug #518 #519

Merged
merged 2 commits into from
Nov 19, 2022
Merged

Fix to the bug #518 #519

merged 2 commits into from
Nov 19, 2022

Conversation

ernstc
Copy link
Contributor

@ernstc ernstc commented Oct 12, 2022

Added check on the model type before it proceeds accessing the property with its getter or setter.
The exception is thrown because it tries to get the value of a property defined on a different type.

Given two commands A and B, where B is sub command of A, bot commands define an argument property. In this case it tries to get the value of the argument property defined in A from the model of type B. This thows the exception described in the bug #518.

@codecov
Copy link

codecov bot commented Oct 22, 2022

Codecov Report

Merging #519 (99c5417) into main (63a43d5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 99c5417 differs from pull request most recent head 1c89f04. Consider uploading reports for the commit 1c89f04 to get more accurate results

@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
+ Coverage   82.28%   82.30%   +0.02%     
==========================================
  Files         106      106              
  Lines        3302     3306       +4     
==========================================
+ Hits         2717     2721       +4     
  Misses        585      585              
Impacted Files Coverage Δ
...neUtils/Conventions/ArgumentAttributeConvention.cs 91.07% <100.00%> (+0.33%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@natemcmaster
Copy link
Owner

Thanks for sending a PR. Would you mind adding a test case? It looks like you basically wrote them in the issue description of #518

@ernstc
Copy link
Contributor Author

ernstc commented Nov 14, 2022

Hi @natemcmaster, I have added the tests relative to the issue.

@natemcmaster
Copy link
Owner

Thanks @ernstc !

@natemcmaster natemcmaster added this to the 4.0.2 milestone Nov 19, 2022
@natemcmaster natemcmaster merged commit 809f8f2 into natemcmaster:main Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception when defining a property as Argument in a command and also in one of its sub commands
2 participants