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 description of collectwavecoeffs #82

Merged
merged 1 commit into from
May 3, 2024

Conversation

vanderhe
Copy link
Member

At the moment the description listed in the help page of the collectwavecoeffs script is wrong, i.e. referring to the skgen description.

Copy link
Member

@bhourahine bhourahine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, but I wasn't able to work out why a local copy of get_parser_and_subparser_container is required (other than possibly different comments/strings)..

@vanderhe
Copy link
Member Author

vanderhe commented May 3, 2024

Seems fine, but I wasn't able to work out why a local copy of get_parser_and_subparser_container is required (other than possibly different comments/strings)..

It seemed to be the established strategy that at least some scripts provide their own definition. Doesn't mean it's a good design per se...

@bhourahine
Copy link
Member

Seems fine, but I wasn't able to work out why a local copy of get_parser_and_subparser_container is required (other than possibly different comments/strings)..

It seemed to be the established strategy that at least some scripts provide their own definition. Doesn't mean it's a good design per se...

OK, but at some point we should discuss the style guide for Python and, as with the pylint stuff in #80, work out something that will be stuck to.

@vanderhe vanderhe merged commit 90a31e0 into dftbplus:main May 3, 2024
2 of 3 checks passed
@vanderhe vanderhe deleted the collectwavecoeffDescr branch May 3, 2024 14:34
@vanderhe vanderhe added the bugfix Fixes a bug label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants