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

Fix CI/CL #1197

Closed
wants to merge 2 commits into from
Closed

Fix CI/CL #1197

wants to merge 2 commits into from

Conversation

WyattBlue
Copy link
Member

No description provided.

@jlaine
Copy link
Member

jlaine commented Oct 28, 2023

Some points:

  • one commit = one change, I'm not fond of "mixed bag" commits which change a whole bunch of things
  • let's try to make commit message descriptive, that's all well have to go one in a year or more to understand why a change was made. "fix ci/cl" doesn't seem descriptive: you're abandoning support for FFmpeg prior to 5.1 aren't you?
  • commented code is a code smell: either we want the code or we don't (we have the git history).
  • I haven't looked into the detail but why comment out a failing test? It used to pass, if it doesn't it's a sign either the test needs redesigning or something has changed under our feet
  • I'm not against style changes but ensure they are enforced by CI using an automated tool, preferably black + ruff

@WyattBlue
Copy link
Member Author

@jlaine Your points are correct. I'm just trying to figure out why the tests are failing, then I'm clean up the PR.

@hmaarrfk
Copy link
Contributor

I came here because I was curious to see the progress.

I learned that it is possible, if you want to have large style changes as this one, to ignore a specific commit in the git blame.

@jlaine
Copy link
Member

jlaine commented Oct 28, 2023

@jlaine Your points are correct. I'm just trying to figure out why the tests are failing, then I'm clean up the PR.

Sounds good! Again kudos for the energy you're putting into this, don't get me wrong I'm very grateful for your involvement :)

@jlaine
Copy link
Member

jlaine commented Oct 29, 2023

AFAICT the test that was "commented" is in fact test_doctests.py which is completely removed? Could we have the rationale for this and have it in a separate commit please?

What tool did you use to reformat the tests? Currently we run black, flake8 and isort (the two last ones might be replaced by ruff if it supports Cython code).

@WyattBlue WyattBlue closed this Oct 29, 2023
@WyattBlue
Copy link
Member Author

black is not being applied to the tests directory.

@WyattBlue WyattBlue deleted the fix-everything branch October 29, 2023 20: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.

3 participants