Skip to content

Fix to the bug #518 #519

Merged
natemcmaster merged 2 commits intonatemcmaster:mainfrom
ernstc:main
Nov 19, 2022
Merged

Fix to the bug #518 #519
natemcmaster merged 2 commits intonatemcmaster:mainfrom
ernstc:main

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.

@ernstc ernstc requested a review from natemcmaster as a code owner October 12, 2022 23:07
@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