Remove obsolete APIs for 3.0 #335
Conversation
|
Thanks for taking on this task, @mattnischan! Have you had a chance to look into the test failures? Looks like there are 16 failures: https://dev.azure.com/natemcmaster/github/_build/results?buildId=765&view=ms.vss-test-web.build-test-results-tab |
|
Something very strange is going on locally with my build. I had all the build errors cleared when I pushed, and tests were working, but checking again now I have a number of new build errors VS hadn't surfaced before. Let me do some digging and see if I can resolve the issues. |
|
@natemcmaster - I think this looks OK now. Got the unit tests squared away and ended up adding an exception specifically for subcommands with no Command attribute. Added a test for that as well. Hopefully that is capturing the correct intent behind the Subcommand/Command API change. |
| var commandAttribute = typeof(TSubCommand).GetTypeInfo().GetCustomAttributes(typeof(CommandAttribute)).FirstOrDefault() as CommandAttribute; | ||
| if(commandAttribute == null) | ||
| { | ||
| throw new CommandAttributeMissingException(typeof(TSubCommand)); |
There was a problem hiding this comment.
This isn’t a behavior I think we should change. The Command attribute is currently optional for subcommand types. Can you find a way to remove the obsolete API without this? I believe it should work if this method is changed to...
context.Application.Command<TSubCommand>(null!, subcommand.Configure);There was a problem hiding this comment.
Not a problem at all. I misunderstood #139 to mean that [Command] would now be required. The subcommand's Configure() only set up the Name field, which has been removed, so I removed that method also. Looks like passing null to both works just fine (there are checks in the code to handle those cases, seems).
| @@ -111,6 +111,7 @@ public void ItDoesNotInitalizeClassUnlessNecessary() | |||
| Assert.Same(app, parseResult.SelectedCommand); | |||
| } | |||
|
|
|||
| [Command] | |||
There was a problem hiding this comment.
Can you undo all the places this had to be added? I’d like to ensure we maintain backwards compatibility as much as possible.
There was a problem hiding this comment.
Removed where added.
…nds. Added test for subcommand naming via the Command attribute.
|
@natemcmaster My pleasure! Thank you so much. |
Addresses #251
Wasn't completely sure how best to deal with the doc changes or the additional exception in the subcommand convention, so let me know if stuff needs to change there.