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

Normalize files #2334

Merged
merged 8 commits into from
May 6, 2024
Merged

Normalize files #2334

merged 8 commits into from
May 6, 2024

Conversation

matrss
Copy link
Collaborator

@matrss matrss commented Apr 22, 2024

Add lint rules to avoid having CRLF endings in the repository and to prevent trailing whitespace at EOL and duplicate newlines at EOF.

Fixes #2330.

@matrss matrss mentioned this pull request Apr 22, 2024
@matrss matrss force-pushed the normalize-files branch 2 times, most recently from 419e332 to df25ea2 Compare April 22, 2024 14:11
matrss added 2 commits April 22, 2024 16:11
Whitespace issues as understood by git (by default) are trailing
whitespace at EOL, whitespace on an otherwise empty line, a space
character immediately followed by a tab character in the indentation of
a line, and multiple newlines at EOF.

References:
- https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---check
- https://peter.eisentraut.org/blog/2014/11/04/checking-whitespace-with-git
@matrss matrss force-pushed the normalize-files branch 5 times, most recently from 17de130 to 18fe4b9 Compare April 22, 2024 15:18
@matrss matrss marked this pull request as ready for review April 22, 2024 15:39
@matrss matrss requested a review from ReimarBauer April 22, 2024 15:40
@matrss
Copy link
Collaborator Author

matrss commented Apr 22, 2024

For a diff of what meaningfully changed you can do git diff stable --ignore-all-space --ignore-blank-lines in a local checkout of this branch.

name: lint

on:
push:
Copy link
Member

Choose a reason for hiding this comment

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

should we add here also new GSOC branches?

Copy link
Collaborator Author

@matrss matrss Apr 29, 2024

Choose a reason for hiding this comment

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

Yes. This is already the case for the python-flake8 workflow on develop, but since this is on stable I missed it. Again, the issue of having two development branches. I'll fix this up. EDIT: actually, this is something that needs to be fixed when merging stable into develop. flake8-builtins is also missing here and needs to be considered when merging.

Copy link
Member

Choose a reason for hiding this comment

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

ok on the next merge of develop to stable we have a new major release and also both branches have a similiar test infrastructure.

And yes stable is the branch we do release from. We have a major develop branch and sometimes based on that other feature branches. That is no issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok on the next merge of develop to stable we have a new major release and also both branches have a similiar test infrastructure.

I am not sure if you misunderstood me. I was talking about the next merge from stable to develop, i.e. what should happen right after this PR has been merged, not on the next merge from develop to stable (a major release). These differences between the python-flake8.yml workflow on stable and the python-flake8.yml workflow on develop need to be accounted for in the merge of stable into develop.

@ReimarBauer
Copy link
Member

sorry for the delay, had also an idea for another check, see #2338

I added few remarks and questions.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

LGTM

@ReimarBauer
Copy link
Member

need to change rules. The flake8 mandatory check job was named lint only.

@matrss matrss merged commit 8b5c734 into stable May 6, 2024
6 checks passed
@matrss matrss deleted the normalize-files branch May 6, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants