Skip to content

Rename CI job names #5618

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

Merged
merged 2 commits into from
Jan 6, 2022
Merged

Rename CI job names #5618

merged 2 commits into from
Jan 6, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Taking inspiration from pip as seen here pypa/pip#10720 I really like their clean CI jobs names.
This is probably personal preference, but would we consider changing them? This is my suggestion based on their design.

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Dec 31, 2021
@coveralls
Copy link

coveralls commented Dec 31, 2021

Pull Request Test Coverage Report for Build 1640380610

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.693%

Totals Coverage Status
Change from base Build 1638793982: 0.0%
Covered Lines: 14336
Relevant Lines: 15301

πŸ’› - Coveralls

@DanielNoord
Copy link
Collaborator Author

We probably need to change some setting about which actions are required for PRs to mergeable for this to work.

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Tbh at first glance I though this looked quite good, but the CI test summery is much more readable with the old style. Could be the multiple slashes that make it more difficult to read.

I would suggest to keep the old style.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM ! Some suggestions

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Dec 31, 2021
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM, but I think it requires that I change the settings of the repositories for required jobs, so it might be a mess during the time where both old job exists on out of date branch and new jobs on up to date branch. Maybe it's time to remove required job because all maintainers are responsible individual that will only merge responsibly anyway ? (Also if Marc think the old one are better and we still require some job to pass, it would creates work for an opinionated change and we may choose to close)

@cdce8p
Copy link
Member

cdce8p commented Jan 1, 2022

Maybe it's time to remove required job because all maintainers are responsible individual that will only merge responsibly anyway ?

I think that should be fine. That had been the default a while back anyway.

(Also if Marc think the old one are better and we still require some job to pass, it would creates work for an opinionated change and we may choose to close)

Just my opinion, in the PR view I definitely think the old style looked better. The only advantage I see is in the actions side panel, but just ever so slightly. I mentioned it earlier already, it could be the multiple slashes for each entry which make it much more difficult to read. Overall I'm somewhere between -0.5 and -1 on this one. However if you both think it's a definite improvement, I guess I could live with it too.

@Pierre-Sassoulas
Copy link
Member

I removed the required job and I think this is making the test name easier to parse by giving predictability. As this is not a very important change let's go with the majority opinion here.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 1d5b3f1 into pylint-dev:main Jan 6, 2022
@DanielNoord DanielNoord deleted the CI branch January 6, 2022 13:05
areveny pushed a commit to areveny/pylint that referenced this pull request Jan 9, 2022
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion πŸ€” Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants