poetry init --author multiple authors fix #8864 #10402
Conversation
…xample.com>' --author 'Authorname2 <[email protected]>'", to test multiple author input, this is a temporary test for now will likely revert later and just add a second --author call to the original test
…uthor name1 --author name2
…out.py file to handle multiple author inputs still
…any other program still calls layouts, with a string
…but now the initial test isnt working for a seemingly unrealated formatting issue with the expected output
…y i can modify the layout code to handle this
…y i can modify the layout code to handle this
2 authors are getting added in the correct format, but I still have some format issues in the testing file expected output but almost there
if author == author_str:
author = ""
This handles if user simply hits enter no input
if author is None:
authors = author
This is if the user enter "n" as other tests error out for author not being set to the defaults instead otherwise this will be set to the vcs_config values
authors = [author] if author else authors
This handles if a user enters 1 author, this will then be added
…and both still work, did not run entire test suite
…eleted an extra line in test_init.py that was flagged as a change from main poetry file.
Reviewer's GuideThis PR converts the single-author init workflow to support multiple --author options end-to-end: the CLI option is updated to collect lists, interactive prompts now display and validate multiple entries, the layout generator normalizes and iterates over author lists when building pyproject.toml, and a new test verifies the behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
kaminskj
changed the title
kaminskj
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kaminskj - I've reviewed your changes - here's some feedback:
- The interactive prompt logic around distinguishing empty input, skip (
n), and updating the authors list is quite complex—consider refactoring it so it clearly handles those three cases without string‐comparison hacks. - You only added a test for two
--authorflags; add a test for three (or more) authors to confirm unlimited multiple‐author support works as expected. - In
init.py, the mix of variables namedauthorandauthorsis confusing—consider a clearer naming scheme or separating singular vs. plural flows to reduce ambiguity.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self._dependencies = dependencies or {} | ||
| self._dev_dependencies = dev_dependencies or {} | ||
|
|
||
| if not author: |
There was a problem hiding this comment.
suggestion (bug_risk): Prefer checking for None over falsy author
Use if author is None: so empty strings or lists aren’t treated as missing.
| if not author: | |
| if author is None: |
|
|
||
| authors = [author] if author else [] | ||
| authors = [author] if author else authors # checks if author = None or "" | ||
| authors = authors if authors else [] |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| authors = authors if authors else [] | |
| authors = authors or [] |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
| version, | ||
| description=description, | ||
| author=authors[0] if authors else None, | ||
| author=authors if authors else None, |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| author=authors if authors else None, | |
| author=authors or None, |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
There was a problem hiding this comment.
Hey @kaminskj - I've reviewed your changes - here's some feedback:
- In layout.py init, use
if author is Noneinstead of a falsy check so that an explicit empty list isn’t overridden by the default author. - The interactive prompt logic in init.py is a bit tangled—consider simplifying how you distinguish between empty input, ‘n’ (skip), and preserving the existing authors array to avoid assigning None or an empty string inadvertently.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| authors = [author] if author else [] | ||
| authors = [author] if author else authors # checks if author = None or "" | ||
| authors = authors if authors else [] |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| authors = authors if authors else [] | |
| authors = authors or [] |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
| version, | ||
| description=description, | ||
| author=authors[0] if authors else None, | ||
| author=authors if authors else None, |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
| author=authors if authors else None, | |
| author=authors or None, |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
…pre-commit. mypy found no issues, pre-commit found issue fix-end-of-file but for files i didnt edit.
…rwritted "N" means set author to the default strings a author nae means set author to the user input
… is_interactive and vcs_config if statements.
ran code linter for new changes, and cleaned up conditional statements. Currenntly pytest ends OK
radoering
left a comment
radoering
left a comment
There was a problem hiding this comment.
So we can set multiple authors via --author but we cannot set them interactively, can we? (Although this is a bit inconsistent, we can also define it as "out of scope" for this PR.)
| question.set_validator(lambda v: self._validate_author(v, authors)) | ||
| author = self.ask(question) | ||
| if author == author_str: # user entered nothing, dont change authors | ||
| author = "" |
There was a problem hiding this comment.
It looks like author is not used after this line. Why do we set it here?
There was a problem hiding this comment.
Why not introduce a new --authors flag for multiple authors?
3 participants
Pull Request Check List
Resolves: #8864
Changed Authors option in init.py to multiple=True. This allows the user to input –author name1 –author name2, to the nth author name. This does change the output of this option from a string to a list, which impacts downstream code.
For init.py changed line 151, author, Is now authors as this variable corresponds to a potential list of authors instead of a string. Since we are now handling a list, line 157 has to change the vcs.config author created into a list, so downstream code will work with it.
Updated the is_interactive string, so multiple authors can be displayed (lines 164 to 168). Lines 166 to 169 handle user inputs to the is_interactive. If the user inputs nothing, then displayed authors should stay the same. If user enters “n” then author reverts to defaults. This is defined in the tests, removing this will cause errors where authors in the final file won't be set to defaults. Lines 172 and 173 handle the original line 165. This may be a different issue, not sure if user entering "n" should mean no to current input or no to entering a different author.
The layout function (248 init.py) now takes an entire list of authors, before it only took index 0. This required changes to layout, as it wasn’t coded to handle multiple author inputs.
Updated layout.py to take list inputs, line64, previously it only took str inputs. Added a handle for if user enters multiple or just 1 author as an --author input (lines 88 to 92 layout.py).
Now when creating the layout, a for loop is needed to iterate over the list of multiple authors the only change is wrapping the existing code handling authors in a for loop iterating over each author in the list (lines 148 to 157 layout.py)
In test_init.py added a new test to check for multiple author inputs. Simply copied test_predefined to create a 2 author input. The only change is line 911 and 912, and then changes to the expected output for the check (lines 926 and 927). The remaining code is copied from test_predefined.
Poetry run pytest successfully completes with only a threading error which I think is due to issues with my local environment setup -> tests/utils/test_threading.py::test_threading_atomic_cached_property_different_instances[value_functools_cache-24] - AssertionError: assert 11 == 24
------ Only Added 1 new test, that adds 2 authors, existing test check for 1 author, did not test 3 or more authors
------Code doesn't require a documentation change. Currently --author command is documented, and it seems the method of adding multiple commands (e.g --author name1 --author name2... --author nameX, is standard for these types of commands) Documentation could be added for extra clarification but doesn't seem required currently.
Summary by Sourcery
Enable passing multiple authors to the
poetry init --authoroption by converting internal author handling from a single string to a list, updating prompts and layout generation accordingly, and adding a validation test.New Features:
--authorflags to specify multiple project authors inpoetry init.Enhancements:
init.pyandlayout.pyto store authors as a list and iterate when building the project configuration.Tests:
test_predefined_2_authorsto verify generation of two authors in the pyproject output.