- Notifications
You must be signed in to change notification settings - Fork 11
Add -i/--issue and -s/--section flags to blurb add#16
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
picnixz commented Jun 25, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
hugovk commented Jun 25, 2024
That could make things easier, but I've not used subcommands much in argparse, how are they? We'd need to make sure all the commands and options work in the same way, blurb is used in a few different bits of automation and we wouldn't want to break any of them. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
picnixz commented Jun 26, 2024
From my personal usage of subcommands, it works the same. Actually, a subcommand is just an action that invokes an ArgumentParser. However, it might indeed come with some corner cases which I would first extensively test. If you can tell me where it's being used in automaton parts, then I'd be glad to check if I can make the transition. |
hugovk commented Jun 26, 2024
Hmm, well at least here: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
larryhastings left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments. I'll confer further with Hugo.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
picnixz commented Jul 12, 2024
Thank you Larry for your comments. I'll address them tomorrow (I don't have access to the project now) |
- remove section IDs matching - do not render the table in case of a multi-match - simplify `add` docstring construction - update tests - update README.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
picnixz commented Jul 13, 2024
I ended up making them match like that (the tests should reflect what is now possible). I don't have a better way to cover the use cases so don't hesistate to tell me which cases should be checked. |
larryhastings commented Jul 13, 2024
Would it be sufficient to create a "nickname map", like
|
picnixz commented Jul 13, 2024
Yes. But to construct additional patterns, I needed to use: _sanitized=re.sub(r'[ /]', ' ', _section) _section_words=re.split(r'\s+', _sanitized) _section_names_lower_nosep[_section] =''.join(_section_words).lower()(Your suggestion is in my case implemented as |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-i/--issue and -s/--section flags to blurb add-i/--issue and -s/--section flags to blurb add
hugovk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully sold on the smart matching, I don't think we need to worry about things like blurb add --section "cOre _ and - bUILtins".
Would case-insensitive substring matching be enough, as long as the substring is unique?
So these would work:
blurb add --section mac blurb add --section core blurb add --section build blurb add --section built But not:
blurb add --section buil blurb add --section i | ifissue.startswith('gh-'): | ||
| issue=issue[3:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ifissue.startswith('gh-'): | |
| issue=issue[3:] | |
| issue=issue.removeprefix('gh-') |
| del_sanitized | ||
| # '_', '-', ' ' and '/' are the allowed (user) separators | ||
| _section_pattern=r'[_\- /]?'.join(map(re.escape, _section_words)) | ||
| # add '$' to avoid matching after the pattern | ||
| _section_pattern=f'{_section_pattern}$' | ||
| del_section_words | ||
| _section_pattern=re.compile(_section_pattern, re.I) | ||
| _section_special_patterns[_section].add(_section_pattern) | ||
| del_section_pattern, _section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these dels?
| del_sanitized | |
| # '_', '-', ' ' and '/' are the allowed (user) separators | |
| _section_pattern=r'[_\- /]?'.join(map(re.escape, _section_words)) | |
| # add '$' to avoid matching after the pattern | |
| _section_pattern=f'{_section_pattern}$' | |
| del_section_words | |
| _section_pattern=re.compile(_section_pattern, re.I) | |
| _section_special_patterns[_section].add(_section_pattern) | |
| del_section_pattern, _section | |
| # '_', '-', ' ' and '/' are the allowed (user) separators | |
| _section_pattern=r'[_\- /]?'.join(map(re.escape, _section_words)) | |
| # add '$' to avoid matching after the pattern | |
| _section_pattern=f'{_section_pattern}$' | |
| _section_pattern=re.compile(_section_pattern, re.I) | |
| _section_special_patterns[_section].add(_section_pattern) |
| del_sec_name_width | ||
| sections_table='\n'.join(map(_format_row, sections)) | ||
| del_format_row | ||
| sections_table='\n'.join((_sec_row_rule, sections_table, _sec_row_rule)) | ||
| del_sec_row_rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| del_sec_name_width | |
| sections_table='\n'.join(map(_format_row, sections)) | |
| del_format_row | |
| sections_table='\n'.join((_sec_row_rule, sections_table, _sec_row_rule)) | |
| del_sec_row_rule | |
| sections_table='\n'.join(map(_format_row, sections)) | |
| sections_table='\n'.join((_sec_row_rule, sections_table, _sec_row_rule)) |
| defcheck(section, expect): | ||
| actual=blurb._extract_section_name(section) | ||
| assertactual==expect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the other test files:
| defcheck(section, expect): | |
| actual=blurb._extract_section_name(section) | |
| assertactual==expect | |
| defcheck(section, expected): | |
| actual=blurb._extract_section_name(section) | |
| assertactual==expected |
| res=blurb._update_blurb_template(issue=None, section=section) | ||
| res=res.splitlines() | ||
| forsection_nameinblurb.sections: | ||
| ifsection_name==expect: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ifsection_name==expect: | |
| ifsection_name==expected: |
| ('section', 'expect'), | ||
| tuple(zip(blurb.sections, blurb.sections)) | ||
| ) | ||
| deftest_exact_names(self, section, expect): | ||
| self.check(section, expect) | ||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ('section', 'expect'), | |
| tuple(zip(blurb.sections, blurb.sections)) | |
| ) | |
| deftest_exact_names(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), [ | |
| ('section', 'expected'), | |
| tuple(zip(blurb.sections, blurb.sections)) | |
| ) | |
| deftest_exact_names(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), [ |
| deftest_partial_words(self, section, expect): | ||
| self.check(section, expect) | ||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| deftest_partial_words(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), [ | |
| deftest_partial_words(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), [ |
| deftest_partial_special_names(self, section, expect): | ||
| self.check(section, expect) | ||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| deftest_partial_special_names(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), [ | |
| deftest_partial_special_names(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), [ |
| deftest_partial_separators(self, section, expect): | ||
| # normalize the separtors '_', '-', ' ' and '/' | ||
| self.check(section, expect) | ||
| @pytest.mark.parametrize( | ||
| ('prefix', 'expect'), [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus a typo
| deftest_partial_separators(self, section, expect): | |
| # normalize the separtors '_', '-', ' ' and '/' | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('prefix', 'expect'), [ | |
| deftest_partial_separators(self, section, expected): | |
| # normalize the separators '_', '-', ' ' and '/' | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('prefix', 'expected'), [ |
| deftest_partial_prefix_words(self, prefix, expect): | ||
| # try to find a match using prefixes (without separators and lowercase) | ||
| self.check(prefix, expect) | ||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), | ||
| [(name.lower(), name) fornameinblurb.sections], | ||
| ) | ||
| deftest_exact_names_lowercase(self, section, expect): | ||
| self.check(section, expect) | ||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), | ||
| [(name.upper(), name) fornameinblurb.sections], | ||
| ) | ||
| deftest_exact_names_uppercase(self, section, expect): | ||
| self.check(section, expect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| deftest_partial_prefix_words(self, prefix, expect): | |
| # try to find a match using prefixes (without separators and lowercase) | |
| self.check(prefix, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), | |
| [(name.lower(), name) fornameinblurb.sections], | |
| ) | |
| deftest_exact_names_lowercase(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), | |
| [(name.upper(), name) fornameinblurb.sections], | |
| ) | |
| deftest_exact_names_uppercase(self, section, expect): | |
| self.check(section, expect) | |
| deftest_partial_prefix_words(self, prefix, expected): | |
| # try to find a match using prefixes (without separators and lowercase) | |
| self.check(prefix, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), | |
| [(name.lower(), name) fornameinblurb.sections], | |
| ) | |
| deftest_exact_names_lowercase(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), | |
| [(name.upper(), name) fornameinblurb.sections], | |
| ) | |
| deftest_exact_names_uppercase(self, section, expected): | |
| self.check(section, expected) |
picnixz commented Jul 1, 2025
I think it would be enough. |
Integrate the user-friendly features from PR python#16 by @picnixz into the automation support from PR python#45, making the CLI more intuitive: - Change --gh-issue to --issue, accepting multiple formats: * Plain numbers: --issue 12345 * With gh- prefix: --issue gh-12345 * GitHub URLs: --issue python/cpython#12345 - Add smart section matching with: * Case-insensitive matching: --section lib matches "Library" * Partial matching: --section doc matches "Documentation" * Common aliases: --section api matches "C API" * Separator normalization: --section core-and-builtins - Improve error messages for invalid sections This combines the automation features from PR python#45 with the interface improvements suggested by @picnixz in PR python#16, as reviewed by @hugovk and @larryhastings. Co-authored-by: picnixz <picnixz@users.noreply.github.com>
Integrate the user-friendly features from PR python#16 by @picnixz into the automation support from PR python#45, making the CLI more intuitive: - Change --gh-issue to --issue, accepting multiple formats: * Plain numbers: --issue 12345 * With gh- prefix: --issue gh-12345 * GitHub URLs: --issue python/cpython#12345 - Add smart section matching with: * Case-insensitive matching: --section lib matches "Library" * Partial matching: --section doc matches "Documentation" * Common aliases: --section api matches "C API" * Separator normalization: --section core-and-builtins - Improve error messages for invalid sections This combines the automation features from PR python#45 with the interface improvements suggested by @picnixz in PR python#16, as reviewed by @hugovk and @larryhastings.
Fixes#6. This is an alternative to #15.
@hugovk I've taken out your way of handling positional arguments (but would you consider a PR which refactors blurb in order to use
argparseinstead?). If you prefer--gh, I can also rename the variable!