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

get ci pipelines functional, and switch to ruff linter/formatter #264

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

rippleFCL
Copy link
Contributor

@rippleFCL rippleFCL commented Mar 22, 2024

Initial Checklist

  • Has @tigattack / @issy been added as a reviewer?
  • If applicable, have the relevant project(s), milestone(s), and label(s) been applied?
  • If applicable, have you added details of the cog to the readme as per README.md?

Details

Does this resolve an issue?
Resolves #

Changes

Features / Fixes

  • Adds feature/fix which does xyz.

Breaking Changes

  • Adds breaking change which causes <issue>.

Additional

Any further notes or comments you want to make.
im cool so i dont need to fill out the form

ripplefcl added 3 commits March 22, 2024 11:38
	modified:   requirements-ci.txt
	modified:   .github/workflows/ci.yml
@tigattack tigattack self-requested a review March 22, 2024 12:06
@tigattack tigattack added the ci label Mar 22, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@issy
Copy link
Member

issy commented Mar 22, 2024

Why autopep8 over black? Is there actually a tangible improvement here?

@rippleFCL
Copy link
Contributor Author

rippleFCL commented Mar 22, 2024

main things id say are, black isnt compatible with pep8 or flake8. it also formats quotes to double quotes and it removes trailing commas. secondly (kinda unrelated) me and tig are thinking abt precommit formatting. currently the repo is more autopep8 compliant than it is black. also if we are using flake8 for linting, precommit formatting with black might cause issues there

@issy
Copy link
Member

issy commented Mar 22, 2024

Yeah fair enough I guess. Sounds good

@issy
Copy link
Member

issy commented Mar 22, 2024

While we're in the area, it might be a good time to version lock the CI dependencies. Reproducible builds are good

@rippleFCL rippleFCL marked this pull request as draft March 23, 2024 02:22
@rippleFCL
Copy link
Contributor Author

so just for comment here i undid the decision on black vs autopep8 after bit more digging the linters can be made to play nice with black, i forgot how much easier black formated code is to read

@rippleFCL rippleFCL marked this pull request as ready for review May 3, 2024 12:49
@rippleFCL rippleFCL changed the title fixing the pylint pipeline and switched linters to autopep8 fixing the pylint pipeline and switched linters to black May 3, 2024
requirements-ci.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
requirements-ci.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rippleFCL rippleFCL changed the title fixing the pylint pipeline and switched linters to black get ci pipelines functional, and switch to ruff linter/formatter May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants