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

Remove Curlylint, add Black and fix ESLint #385

Merged
merged 8 commits into from
Oct 3, 2024

Conversation

CAM-Gerlach
Copy link
Member

Pull Request

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 4.x)
  • Checked your writing carefully for correct English spelling, grammar, etc
  • Described your changes and the motivation for them below

Description of Changes

Followup from #383 and #384 , removes the unused, unmaintained and slow Jinja template linter Curlylint, adds the code formatter Black and applies the latter to blacken the repo's Python scripts (excluding the examples in the /workshops subdirectory). Additionally, adds a custom .black.toml config file to avoid adding an unnecessary, inappropriate and potentially misleading pyproject.toml to this repo that's not any form of Python package.

@CAM-Gerlach CAM-Gerlach self-assigned this Sep 25, 2024
@CAM-Gerlach
Copy link
Member Author

Seems like there's a strange, hopefully temporary dependency conflict issue trying to install eslint (never seen that before), so hopefully that'll result itself in a day or two; there's no rush to merge this immediately.

@CAM-Gerlach
Copy link
Member Author

Welp, seems like the strange NPM dependency conflict that suddenly popped up has still not been resolved, so I'll dig in and investigate it myself.

@CAM-Gerlach
Copy link
Member Author

Welp, after several hours investigating, I reported this as eslint/eslint#18956 , and appears to be solved by eslint/eslint#18945 and was able to drill down to a workaround for now. Unfortunately, pinning the specific problematic dep didn't work for some reason, so I had to pin ESLint on a modestly older version for now until that's released.

@CAM-Gerlach CAM-Gerlach changed the title Remove Curlylint, add Black and blacken repo scripts Remove Curlylint, add Black and fix ESLint Sep 26, 2024
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I think one thing is missing before merging: we should add commit 9e75d18 to .git-blame-ignore-revs so that it's ignored by git blame.

That's the recommended approach when applying Black to a repo and I think it's a good idea.

@CAM-Gerlach
Copy link
Member Author

Okay, sure; I didn't add that before since the blackening only affects a couple scripts, and users need to either manually pass the path to that file every time, or (in recent-ish Git versions) manually configure Git to set and use this as the ignore file. However, after thinking about it a bit, I realized I can just add that to the things the setup command does, which I did so.

I also noticed and fixed a small perf issue in setup-remotes.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @CAM-Gerlach!

@ccordoba12 ccordoba12 merged commit fa91f0e into spyder-ide:master Oct 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants