[Fix] --help command #884
[Fix] --help command #884
Conversation
| @@ -37,13 +40,15 @@ class InvalidConfigFile(Exception): | |||
| """ In place of YAMLError | |||
| """ | |||
|
|
|||
| def __new__( cls, parser: ArgumentParser = None, strict: bool = False ): | |||
| def __new__( cls, parser: ArgumentParser = None, strict: bool = False, args: Optional[List[str]] = None ): | |||
There was a problem hiding this comment.
Is there a reason for this change? I will be hesitant to change the base config class unless necessary. Every single bittensor object initiates a base config object. So ideally, we should keep it as lightweight as possible. The config class is just a whole bag of worms XD
There was a problem hiding this comment.
I also think the config class is in need of a cleanup, so it might be good for the next update.
There was a problem hiding this comment.
this is an inversion of control of the sys.argv from the command line.
Normally argparser gets it internally, if you don't specify which args to parse, by calling sys.argv the same way I added in the neuron __main__
I added it as an optional arg and lower down it defaults to calling sys.argv if None, so the class will function as normal without the arg specified.
The IoC is important for testing if btcli --help works correctly and has the text we expect. Otherwise I would have to patch argparser, which is subject to API changes. This is recommended from some SO posts I read.
Edited: dep injection -> IoC
There was a problem hiding this comment.
Since this sounds like a temporary change before the cleanup, i would generate some issues in this repository to not forget about the tech debt we have around this code.
There was a problem hiding this comment.
I do not know much this codebase, but from my point of view this change seems simple enough to be able to merge it.
It looks direct and effective with its purpose.
I understand that changing functions' interfaces is not the best and can bring chaos to the codebase.
I would like that before we close this PR we create issues tagged as tech-debt exposing the debt we have related with CLI/Config/Args. Because, from my point of view, seems like the bug was also introduced for the existence of the tech-spec.
Finally, since this is going to the nobunaga branch and with no release deadline I see we can merge this and continue learning and improving the codebase from this point if something fails again. By the way, we should have the necessary test coverage for this, do we have it?
This PR:
btcli --helpoutput