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

Add --no-line-numbers option, default false (no behavior change). #98

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

mauritsvanrees
Copy link
Member

Fixes #77

Note for reviewers: a few related test classes had some lines in setUp and then one test_read method. I have merged these methods and then duplicated them into test_read_no_line_numbers and test_read_with_line_numbers. Very little of the setUp was useful for sharing.

@mauritsvanrees mauritsvanrees requested a review from erral September 6, 2022 21:36
Copy link
Member

@erral erral left a comment

Choose a reason for hiding this comment

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

LGTM.

Perhaps we can improve the parameter name. "No line numbers false" is a bit hard to understand because of the double negation. Perhaps it could be better "Remove line numbers" .

But that's just my opinion 😄

In the code translate this and the `--no-line-numbers` option to `include_line_numbers`.
This avoids double negatives, like 'no line numbers is false'.
@mauritsvanrees
Copy link
Member Author

Perhaps we can improve the parameter name. "No line numbers false" is a bit hard to understand because of the double negation. Perhaps it could be better "Remove line numbers" .

Fair point. I had a doubt about the naming as well.
I have refactored it. On the command line you now have two options: --no-line-numbers and --line-numbers. In the Python code I translate this to one parameter include_line_numbers, which by default is True. If you somehow specify both options on the command line, the last one wins.
Is this better?

@erral
Copy link
Member

erral commented Sep 19, 2022

Yes, that's right, way better.

@mauritsvanrees mauritsvanrees merged commit d96434d into master Sep 19, 2022
@mauritsvanrees mauritsvanrees deleted the maurits-no-line-numbers branch September 19, 2022 09:17
@mauritsvanrees
Copy link
Member Author

I have released i18ndude 5.5.0 and have updated coredev 5.2 and 6.0 to use it.
I will leave it up to you to change the coredev scripts to use the new option.

@erral
Copy link
Member

erral commented Sep 19, 2022 via email

@mauritsvanrees
Copy link
Member Author

For me, the main reason to add this option, is to avoid getting merge conflicts when one person works on translations for a language and someone else runs i18ndude and line numbers have changed.
But I will leave it up to you.

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.

Add option to not add line numbers
2 participants