Conversation
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. |
Hmm, well at least here: |
larryhastings
left a comment
There was a problem hiding this comment.
Some initial comments. I'll confer further with Hugo.
|
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
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
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. |
|
Would it be sufficient to create a "nickname map", like
|
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 |
-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.
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
| if issue.startswith('gh-'): | ||
| issue = issue[3:] |
There was a problem hiding this comment.
| if issue.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.
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.
| 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)) |
| def check(section, expect): | ||
| actual = blurb._extract_section_name(section) | ||
| assert actual == expect |
There was a problem hiding this comment.
To match the other test files:
| def check(section, expect): | |
| actual = blurb._extract_section_name(section) | |
| assert actual == expect | |
| def check(section, expected): | |
| actual = blurb._extract_section_name(section) | |
| assert actual == expected |
| res = blurb._update_blurb_template(issue=None, section=section) | ||
| res = res.splitlines() | ||
| for section_name in blurb.sections: | ||
| if section_name == expect: |
There was a problem hiding this comment.
| if section_name == expect: | |
| if section_name == expected: |
| ('section', 'expect'), | ||
| tuple(zip(blurb.sections, blurb.sections)) | ||
| ) | ||
| def test_exact_names(self, section, expect): | ||
| self.check(section, expect) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), [ |
There was a problem hiding this comment.
| ('section', 'expect'), | |
| tuple(zip(blurb.sections, blurb.sections)) | |
| ) | |
| def test_exact_names(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), [ | |
| ('section', 'expected'), | |
| tuple(zip(blurb.sections, blurb.sections)) | |
| ) | |
| def test_exact_names(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), [ |
| def test_partial_words(self, section, expect): | ||
| self.check(section, expect) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), [ |
There was a problem hiding this comment.
| def test_partial_words(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), [ | |
| def test_partial_words(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), [ |
| def test_partial_special_names(self, section, expect): | ||
| self.check(section, expect) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), [ |
There was a problem hiding this comment.
| def test_partial_special_names(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), [ | |
| def test_partial_special_names(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), [ |
| def test_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.
Plus a typo
| def test_partial_separators(self, section, expect): | |
| # normalize the separtors '_', '-', ' ' and '/' | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('prefix', 'expect'), [ | |
| def test_partial_separators(self, section, expected): | |
| # normalize the separators '_', '-', ' ' and '/' | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('prefix', 'expected'), [ |
| def test_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) for name in blurb.sections], | ||
| ) | ||
| def test_exact_names_lowercase(self, section, expect): | ||
| self.check(section, expect) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ('section', 'expect'), | ||
| [(name.upper(), name) for name in blurb.sections], | ||
| ) | ||
| def test_exact_names_uppercase(self, section, expect): | ||
| self.check(section, expect) |
There was a problem hiding this comment.
| def test_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) for name in blurb.sections], | |
| ) | |
| def test_exact_names_lowercase(self, section, expect): | |
| self.check(section, expect) | |
| @pytest.mark.parametrize( | |
| ('section', 'expect'), | |
| [(name.upper(), name) for name in blurb.sections], | |
| ) | |
| def test_exact_names_uppercase(self, section, expect): | |
| self.check(section, expect) | |
| def test_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) for name in blurb.sections], | |
| ) | |
| def test_exact_names_lowercase(self, section, expected): | |
| self.check(section, expected) | |
| @pytest.mark.parametrize( | |
| ('section', 'expected'), | |
| [(name.upper(), name) for name in blurb.sections], | |
| ) | |
| def test_exact_names_uppercase(self, section, expected): | |
| self.check(section, expected) |
|
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!