Adds code clean up #330
Adds code clean up #330
Conversation
| self.write_config_file(namespace, output_file_paths, exit_after=True) | ||
| return namespace, unknown_args | ||
|
|
||
| def get_source_to_settings_dict(self): |
There was a problem hiding this comment.
This method has been part of the configargparse API for a long time. Why delete it? How do you know that no other project relies on it?
There was a problem hiding this comment.
i don't, it's not public, there are no tests for it, and it's not used anywhere in the project.
i can add it back in and add tests and docs
There was a problem hiding this comment.
Sounds good, thanks. I'm seeing occasional references to it @
https://github.com/search?q=get_source_to_settings_dict&type=code
Also, what do you mean by not public?
There was a problem hiding this comment.
As in something they have to have "found", it's not in any doc. This is not part of the original argparse
configargparse.py
Outdated
| # is_required=True and user doesn't provide it. | ||
| def error_method(self, message): | ||
| pass | ||
| del message |
There was a problem hiding this comment.
What is the purpose of this and the other 'del' statements?
There was a problem hiding this comment.
makes the linters happy; a common practice to the "variable defined but not used"
There was a problem hiding this comment.
I've never seen this before and find it to be confusing because it's a no-op and serves no obvious purpose.
It's like seeing a = a; and trying to figure out why somebody would put that in the code.
Unless I'm missing some practical reason for these del statements, I would vote for removing them.
There was a problem hiding this comment.
You're absolutely right that del message is a no-op in terms of runtime behavior—it doesn't optimize memory or change functionality in this context. The reason I included it was to satisfy linters like flake8 or pylint, which flag unused arguments. It’s a common pattern when we want to acknowledge a variable without using it, particularly in libraries or base classes.
The big advantage, which is why I don't want to turn this off in the linters, is that it warns you about a function that is using a function that is not used, this I think is invaluable. For this particular case, this is a base class that doesn't implement anything and only is defining the signature for others to follow.
The solutions are as follow:
def error_method(self, message): # noqa # pylint: disable=unused-argument
pass
Which I don't like because, comments get a little hectic, plus if you have another variable, you don't know which you're ignoring and which youre not, thus I prefer to make it explicit with the del.
Does that make sense?
It's like seeing a = a; and trying to figure out why somebody would put that in the code.
a = a will fail, more like a = None -- but del tells the reader: this is not important, "delete it" from your mind 😇
There was a problem hiding this comment.
Thanks for the explanation. I understand what you're saying about the pros, but I would only support
del message if every line like that also includes a detailed comment about why that line is there - something like
del message # this line doesn't do anything, and is only here to satisfy linters. See discussion in PR #330
There was a problem hiding this comment.
I can live with that, cool! 👍
There was a problem hiding this comment.
same thing for message = None (assuming that would have the same effect on linters)
There was a problem hiding this comment.
If I'm the reader, del tells me to scratch my head and furrow my brow.
|
None of the changes in this PR make sense to me |
2 participants
Please squash
Removes dead code; I ran vulture on the code and these are the ones that stood out the most.