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

First crack at default config for ruff #254

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

First crack at default config for ruff #254

wants to merge 22 commits into from

Conversation

ljstella
Copy link
Contributor

@ljstella ljstella commented Aug 22, 2024

Creating first rough draft PR after conversation w/ Eric about getting this started in a new branch.

Rough draft of the default settings and whatnot for implementing ruff as a linter & formatter. We'll need to take some time to go through the formatting, and the lints, and see what we do and don't want to turn on.

TODO:

  • Confirm current settings, making changes if required
  • Add github ci workflow to format & lint (without applying fixes, and failing on jobs that don't pass?)
  • Add precommit hook

@ljstella ljstella self-assigned this Aug 22, 2024
@ljstella ljstella changed the title First crack at default config First crack at default config for ruff Aug 22, 2024
pyproject.toml Outdated
line-length = 88
indent-width = 4

# Assume Python 3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this 3.11, since that is currently the minimum version required for installing contentctl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we update the comment too to reflect 3.11? I know the version itself has been changed already

Copy link
Collaborator

@cmcginley-splunk cmcginley-splunk left a comment

Choose a reason for hiding this comment

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

Super stoked to see us adding a linter! And love throwing a little rust tool in the mix

pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated
line-length = 88
indent-width = 4

# Assume Python 3.8
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we update the comment too to reflect 3.11? I know the version itself has been changed already

@ljstella
Copy link
Contributor Author

@cmcginley-splunk @pyth0n1c

I've added a pre-commit hook & CI job to this PR. If you want to review the output of the lint job you can see the current errors, or you can run them locally to see the output. I have the pre-commit hook set to automatically fix lints when possible. Not all of our existing lint issues are auto-fixable, but the vast majority are.

I am considering adding E721 to the exclusion list, or at least inline-excluding the several issues in contentctl.py and would like your thoughts on this.

@pyth0n1c in particular- I've not committed the mass-format and mass-lint-fix changes to this branch, you're more than welcome to add them if you'd like, I imagine you want to wrap some of the other existing open PRs so we don't create a massive merge-conflict headache on every single open PR.

@ljstella ljstella marked this pull request as ready for review September 12, 2024 14:40
@pyth0n1c
Copy link
Contributor

See the following first pass PR at resolving these:
#289

Should we hold off on all these changes until we clear the PR backlog?

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