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

gh-6: add -i/--issue and -s/--section flags to blurb add #16

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

picnixz
Copy link

@picnixz picnixz commented Jun 25, 2024

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 argparse instead?). If you prefer --gh, I can also rename the variable!

@hugovk
Copy link
Member

hugovk commented Jun 25, 2024

would you consider a PR which refactors blurb in order to use argparse instead?

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.

src/blurb/blurb.py Outdated Show resolved Hide resolved
src/blurb/blurb.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/blurb/blurb.py Outdated Show resolved Hide resolved
tests/test_blurb_add.py Outdated Show resolved Hide resolved
@picnixz
Copy link
Author

picnixz commented Jun 26, 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.

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
Copy link
Member

hugovk commented Jun 26, 2024

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:

tests/test_blurb_add.py Outdated Show resolved Hide resolved
tests/test_blurb_add.py Outdated Show resolved Hide resolved
tests/test_blurb_add.py Outdated Show resolved Hide resolved
@hugovk hugovk added the enhancement New feature or request label Jun 26, 2024
Copy link
Contributor

@larryhastings larryhastings left a 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.

README.md Outdated Show resolved Hide resolved
src/blurb/blurb.py Outdated Show resolved Hide resolved
src/blurb/blurb.py Outdated Show resolved Hide resolved
src/blurb/blurb.py Outdated Show resolved Hide resolved
@picnixz
Copy link
Author

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
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/blurb/blurb.py Outdated Show resolved Hide resolved
picnixz and others added 3 commits July 13, 2024 11:32
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
src/blurb/blurb.py Outdated Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Testing matching the sections, blurb add --section core works, but not:

blurb add --section built
Invalid section name: 'built'

Choose from the following table:

+-------------------+
| Security          |
| Core and Builtins |
| Library           |
| Documentation     |
| Tests             |
| Build             |
| Windows           |
| macOS             |
| IDLE              |
| Tools/Demos       |
| C API             |
+-------------------+

Or:

blurb add --section api
Invalid section name: 'api'

Choose from the following table:

+-------------------+
| Security          |
| Core and Builtins |
| Library           |
| Documentation     |
| Tests             |
| Build             |
| Windows           |
| macOS             |
| IDLE              |
| Tools/Demos       |
| C API             |
+-------------------+

Should they?

README.md Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@picnixz
Copy link
Author

picnixz commented Jul 13, 2024

Mmmh, I think I only matched from the beginning, not inside. Otherwise, you write 'l' and have a lot of matches (library, core and builtins, tools, etc). And I didn't want to special cases. But maybe we can use aliases? I don't remember now if 'capi' works (but maybe it should)

@hugovk
Copy link
Member

hugovk commented Jul 13, 2024

Mmmh, I think I only matched from the beginning, not inside. Otherwise, you write 'l' and have a lot of matches (library, core and builtins, tools, etc).

Yeah, that's not so useful.

And I didn't want to special cases. But maybe we can use aliases? I don't remember now if 'capi' works (but maybe it should)

"capi" doesn't work:

blurb add --section capi
Invalid section name: 'capi'

Choose from the following table:

+-------------------+
| Security          |
| Core and Builtins |
| Library           |
| Documentation     |
| Tests             |
| Build             |
| Windows           |
| macOS             |
| IDLE              |
| Tools/Demos       |
| C API             |
+-------------------+

I don't think we necessarily need to match "builtins" -> "Core and Builtins" or "demo" -> "Tools/Demos", but "capi" -> "C API" would help, and maybe also "api" -> "C API"?

@picnixz
Copy link
Author

picnixz commented Jul 13, 2024

I don't think we necessarily need to match "builtins" -> "Core and Builtins" or "demo" -> "Tools/Demos", but "capi" -> "C API" would help, and maybe also "api" -> "C API"?

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
Copy link
Contributor

Would it be sufficient to create a "nickname map", like

{ "".join(section.split()).lower(): section for section in sections }

@picnixz
Copy link
Author

picnixz commented Jul 13, 2024

{ "".join(section.split()).lower(): section for section in sections }

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 ''.join(_section_words).lower())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --gh and --section flags to "blurb add"
3 participants