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.

Sign up for GitHub

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

Jump to bottom

Refactor issue_key function to sort issues in a human-friendly way #608

Merged
merged 14 commits into from
Jun 13, 2024
Merged

Refactor issue_key function to sort issues in a human-friendly way #608

merged 14 commits into from
Jun 13, 2024

Conversation

SmileyChris
Copy link
Contributor

@SmileyChris SmileyChris commented Jun 5, 2024 •
edited
Loading

Description

In the issue key sorting method, there's this comment:

        # Maybe we should sniff strings like "gh-10" -> (10, "gh-10")?

Let's do that.

To be more backwards compatible, we'll continue putting just plain numbers last. In reality, it's very unlikely a project is mixing issue types, but if they are, then they'll be grouped:

>>> sorted(["2", "#11", "#3", "gh-10", "gh-4", "omega", "alpha"], key=issue_key)
['alpha', 'omega', '#3', '#11', 'gh-4', 'gh-10', '2']

Checklist

  • Make sure changes are covered by existing or new tests.
  • For at least one Python version, make sure local test run is green.
  • Create a file in src/towncrier/newsfragments/. Describe your
    change and include important information. Your change will be included in the public release notes.
  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).

@SmileyChris SmileyChris requested a review from a team as a code owner June 5, 2024 03:29
adiroiban
adiroiban approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good. Only minor comments.

src/towncrier/test/test_builder.py Outdated Show resolved Hide resolved
src/towncrier/test/test_builder.py Outdated Show resolved Hide resolved
src/towncrier/newsfragments/608.feature.rst Outdated Show resolved Hide resolved
src/towncrier/_builder.py Outdated Show resolved Hide resolved
adiroiban
adiroiban reviewed Jun 10, 2024
View reviewed changes
src/towncrier/test/test_builder.py

def test_ordering(self):
"""
Issues are ordered first by the non-text component, then by their number.
Copy link
Member

Choose a reason for hiding this comment

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

I guess here "issues" means "news fragments"

Suggested change
Issues are ordered first by the non-text component, then by their number.
News fragments are ordered first by the non-text component, then by their number.

adiroiban
adiroiban approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

I left a few inline comments.

I think that part of my confusion is the use of the term "issue" in this PR.

I am not sure what an "issue" is: news fragment vs ticket ID

I think it helps to use consistent terminolgy across the code base and the documentation.

I am not a big fan of "news fragment" ... but this is what we have and I think that we should continue to use it.

Thanks again!

@SmileyChris
Copy link
Contributor Author

I am not sure what an "issue" is: news fragment vs ticket ID

@adiroiban I actually decided to switch to issue rather than ticket. My logic is it's the only term actually used in code and changing it for consistency would be backwards incompatible.

We use the issue_format configuration option and the issues_by_category context argument when rendering.

Even though it's not your preference, are you ok with using "issue" across the board instead of ticket?

@adiroiban
Copy link
Member

Thanks for the update

Even though it's not your preference, are you ok with using "issue" across the board instead of ticket?

Sure. No problem.

Your suggestion is much bettter.

Happy to see some improvements here

Thanks

adiroiban
adiroiban approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for the update. All good!

@SmileyChris SmileyChris mentioned this pull request Jun 13, 2024
Open
@SmileyChris SmileyChris merged commit be32e6b into twisted:trunk Jun 13, 2024
16 checks passed
@SmileyChris SmileyChris deleted the Better-issue-sorting branch June 13, 2024 21:56
SmileyChris added a commit to SmileyChris/towncrier that referenced this pull request Jun 13, 2024
@SmileyChris @pre-commit-ci @adiroiban
Refactor issue_key function to sort issues in a human-friendly way (t…
864f925 
…wisted#608)

* Refactor issue_key function to sort issues in a human-friendly way

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Rename newsfragment

* Small improvement to test to show how text with numeric issues are sorted

* Update src/towncrier/_builder.py docstring grammar

Co-authored-by: Adi Roiban <[email protected]>

* clarify new behaviour in newsfragment

* Add some docstrings/comments to tests

* linelength fix

* Clarify news fragments vs tickets

Co-authored-by: Adi Roiban <[email protected]>

* Consistent use of "issue" rather than "ticket"

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* typo

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adi Roiban <[email protected]>
SmileyChris added a commit that referenced this pull request Jun 14, 2024
@SmileyChris @pre-commit-ci @adiroiban
Orphans in non showcontent categories (#612)
09819ac 
* Always content for orphans, since they don't have a concept of tickets

* Add tests for orphans in non showcontent categories

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add newsfragment

* Add a note in the configuration docs about the change to orphan fragments in non-showcontent categories

* Use rst admonitions for the showcontent notes

* Refactor rendering of title via `config.title_format` (#610)

* Refactor rendering of title via config.title_format

* Add newsfragment

* Config docs

* Fix restructuredtext formatting error

* Refactor issue_key function to sort issues in a human-friendly way (#608)

* Refactor issue_key function to sort issues in a human-friendly way

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Rename newsfragment

* Small improvement to test to show how text with numeric issues are sorted

* Update src/towncrier/_builder.py docstring grammar

Co-authored-by: Adi Roiban <[email protected]>

* clarify new behaviour in newsfragment

* Add some docstrings/comments to tests

* linelength fix

* Clarify news fragments vs tickets

Co-authored-by: Adi Roiban <[email protected]>

* Consistent use of "issue" rather than "ticket"

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* typo

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adi Roiban <[email protected]>

* variable now called issue instead of ticket

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adi Roiban <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiroiban adiroiban adiroiban approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SmileyChris @adiroiban