-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 python linter inconsistent behaviour with quotes #1112
Fix python linter inconsistent behaviour with quotes #1112
Conversation
Ugh this is painful 🙃 If we do all the files at once, like in this PR, it will cause a lot of merge conflict pain. In the past, we've only enforced the linter on changed files, which let us do a gradual roll out--is that possible here? |
Yeah that's my concern too.
That will make the same amount of pain, and let the pain last longer. It will also make a bunch of PRs hard to review. The burden is shifted from developers to reviewers. Plus, developers concurrently working on same file(s) would still need to resolve conflicts. |
…and All-Hands-AI#1100 with the hope to fix the inconsistent behaviour across linters and environments. However, we recently found out that double-quote-string-fixer plugin in pre-commit-hook has inconsistent behaviour on python 3.11 and 3.12. See discussion here. This is sad because while this plugin enforces single quote behaviour with 3.11, it doesn't always enforce so with 3.12. Specifically, with fstr syntax, this plugin allows both single quotes and double quotes with python 3.12. The problem is, some developers have black linter installed/integrated with their IDE, which is probably the most popular linter in python world (ranked by GitHub stars). This linter insists on always using double quotes. Now we have black and double-quote-string-fixer fight each other (iff the developer uses python 3.12) for some quotes (fstr syntax). After a lot of research, I couldn't find a way to enforce single quote behaviour without introducing a new dependency, flake8, together with a plugin for it to enforce quotes' behavior. I believe it's better off introducing the more popular black if we have to introduce a new linter. Since black and autopep8 sometimes fight each other, and they mostly overlap, I further remove autopep8. The unfortunate consequence of this PR is that I had to revert all single quotes back to double quotes. This might cause some inconvenience to existing PRs as they have to resolve conflicts, but I believe the headache will be gone soon. That being said, I am open to abandon this PR if anyone has a better idea to solve the headache.
a11a784
to
cd5160e
Compare
Maybe I shall continue to enforce the current behaviour to avoid annoying merge conflicts - using single quotes over double quotes. I think flake8 can do that. |
I understand your frustration 😭 but it is what it is. In hindsight, we should force that in CI as soon as the community decides a format to use. The later, the larger resistance and pain. What I am gonna do now:
If the above attempt works, then we can enforce a consistent behaviour and do minimal abruption to existing works. If we want to start using black, IMHO a maintainer shall help existing open PRs to resolve the conflicts to make everyone happy (I don't have write access). |
IMO, we need to add |
Good idea. I imagine that would be helpful to those who (for some reason) disable the pre-commit-hook. Should be an one-liner change. Let me quickly do that. Thx! |
* lint-frontend Files.tsx (All-Hands-AI#1089) for lint-frontend exhaustive deps Co-authored-by: Robert Brennan <accounts@rbren.io> * Add build-frontend to build (All-Hands-AI#1137) * build: fix workspace variable name in dev setup (All-Hands-AI#1138) * Revamp docker build process (All-Hands-AI#1121) * refactor docker building * change to buildx * disable branch filter * disable tags * matrix for building * fix branch filter * rename workflow * sanitize ref name * fix sanitization * fix source command * fix source command * add push arg * enable for all branches * logs * empty commit * try freeing disk space * try disk clean again * try alpine * Update ghcr.yml * Update ghcr.yml * move checkout * ignore .git * add disk space debug * add df h to build script * remove pull * try another failure bypass * remove maximize build space step * remove df -h debug * add no-root * multi-stage python build * add ssh * update readme * remove references to config.toml * fix(): build out opendevin modal component (All-Hands-AI#1141) * corrected port (All-Hands-AI#1159) * fix All-Hands-AI#1028, /select-file api call on deleted file in Code Editor caused Error (All-Hands-AI#1158) * fix: /select-file on deleted file exception, detail: All-Hands-AI#1028 * fix: lint. * fix: lint. --------- Co-authored-by: aaren.xzh <aaren.xzh@antfin.com> * Remove custom signal handlers (All-Hands-AI#1153) * (fix) OpenDevin works on OpenDevin issues (All-Hands-AI#1149) * Use SANDBOX_TYPE=exec, use docker image * Use updated image * fix: fix multiple frontend warnings (All-Hands-AI#1143) * test(frontend): add unit tests (All-Hands-AI#1076) * test(frontend): add unit tests for getCachedConfig * test(frontend): add unit tests for getCachedConfig * add unit test for the useTypingEffect hook * add unit test for the useInputComposition hook * create unit test for auth service * remove outdated and failing component test * create unit test for session service * break down saveSettings into smaller functions for testability and create unit test for new mergeAndUpdateSettings --------- Co-authored-by: Robert Brennan <accounts@rbren.io> * Logging additions and fixes (All-Hands-AI#1139) * Refactor print_to_color into a color formatter misc fixes catch ValueErrors and others from Router initialization add default methods * Tweak console log formatting, clean up after rebasing exceptions out * Fix prompts/responses * clean up * keep regular colors when no msg_type * fix filename * handle file log first * happy mypy * ok, mypy --------- Co-authored-by: Robert Brennan <accounts@rbren.io> * fix: make typing chat bubble adhere to max width (All-Hands-AI#1142) * fix: fix delete messages endpoint url (All-Hands-AI#1140) Co-authored-by: Robert Brennan <accounts@rbren.io> * Run docker publish on tags (All-Hands-AI#1162) * run ghcr on tags * Update .github/workflows/ghcr.yml Co-authored-by: RaGe <foragerr@users.noreply.github.com> --------- Co-authored-by: RaGe <foragerr@users.noreply.github.com> * Document LLM prompt and response logging. (All-Hands-AI#1166) * Refactor actions a bit (All-Hands-AI#1165) * refactor to action_manager * make all actions awaitable * move task logic into tasks * Update opendevin/action/tasks.py * feat: Improve docker compose services integration * feat: Add ollama, support, memGPT services * feat: Configure OpenDevin UI container * feat: Docker services * hotfix: Add OS locales * Merge develop into clean-install * Add ollama, support, memGPT services * feat: Docker services * hotfix: Backend app service dependencies fix under Conda hotfix: Frontend integration hotfix: Restore useTranslation() * feat: Add API startup script hotfix: Integration fix * feat: Add FastAPI server and Vite dev server logging for debug and live modes * hotfix: Frontend and API integration. Build improvements. * Add ollama, support, memGPT services * feat: Docker services hotfix: Add OS locales hotfix: Fix start configuration hotfix: Temp fix of build errors feat: Configure OpenDevin UI container hotfix: Run OpenDevin application container chore: Update README feat: Docker Configuration for backend services feat: Ubuntu 20.04 images with CUDA and Miniconda3 refactor: Update docker environment configuration feat: Decouple UI and Python app into services. Co-authored-by: Jim Su <jimsu@protonmail.com> * hotfix: Frontend integration * hotfix: Update Miniconda source package and SHA256 hash * hotfix: Make ESLint 'no-duplicate-imports' to report warnings instead of errors * chore: Arrange '.env' and '.env.dist' files * chore: Rename 'nginx.conf' to 'nginx.conf.dist' * chore: Update Docker services configuration * feat: Add developer.Dockerfile to Devin web UI Docker config. Move Dockerfile to end-uder.Dockerfile * feat: Update NGINX configuration with HTTP/HTTPS server config * feat: Update Devin API host and port * chore: Remove unnecessary docker-compose.yml file version tag * chore: Revert 'packageManager' to pnpm * feat: Add frontend port EXPOSE * feat: Add API ports EXPOSE directives * hotfix: Path to 'env_debug' script * feat: Add developer.Dockerfile * chore: Synchronize with OpenDevin main branch * replace Jest with Vitest as the frontend test runner (All-Hands-AI#1163) * Fix python linter inconsistent behaviour with quotes (All-Hands-AI#1112) * This has been a headache for a long time, and we had All-Hands-AI#1071 and All-Hands-AI#1100 with the hope to fix the inconsistent behaviour across linters and environments. However, we recently found out that double-quote-string-fixer plugin in pre-commit-hook has inconsistent behaviour on python 3.11 and 3.12. See discussion here. This is sad because while this plugin enforces single quote behaviour with 3.11, it doesn't always enforce so with 3.12. Specifically, with fstr syntax, this plugin allows both single quotes and double quotes with python 3.12. The problem is, some developers have black linter installed/integrated with their IDE, which is probably the most popular linter in python world (ranked by GitHub stars). This linter insists on always using double quotes. Now we have black and double-quote-string-fixer fight each other (iff the developer uses python 3.12) for some quotes (fstr syntax). After a lot of research, I couldn't find a way to enforce single quote behaviour without introducing a new dependency, flake8, together with a plugin for it to enforce quotes' behavior. I believe it's better off introducing the more popular black if we have to introduce a new linter. Since black and autopep8 sometimes fight each other, and they mostly overlap, I further remove autopep8. The unfortunate consequence of this PR is that I had to revert all single quotes back to double quotes. This might cause some inconvenience to existing PRs as they have to resolve conflicts, but I believe the headache will be gone soon. That being said, I am open to abandon this PR if anyone has a better idea to solve the headache. * Remove black * Prevent black from changing quotes * Use flake8 to enforce single quotes * Fix quotes in config.py * Add back autopep8 * Add make lint to run linters * hotfix: PR fix * Hotfix/clean install (#21) * hotfix: Added missing variables. Removed .env from repo --------- Co-authored-by: 808vita <97225946+808vita@users.noreply.github.com> Co-authored-by: Robert Brennan <accounts@rbren.io> Co-authored-by: RaGe <foragerr@users.noreply.github.com> Co-authored-by: Alex Bäuerle <alex@a13x.io> Co-authored-by: Akki <akhilvc10@gmail.com> Co-authored-by: மனோஜ்குமார் பழனிச்சாமி <smartmanoj42857@gmail.com> Co-authored-by: Xia Zhenhua <zhhuaxia@gmail.com> Co-authored-by: aaren.xzh <aaren.xzh@antfin.com> Co-authored-by: Boxuan Li <liboxuan@connect.hku.hk> Co-authored-by: sp.wack <83104063+amanape@users.noreply.github.com> Co-authored-by: Engel Nyst <enyst@users.noreply.github.com> Co-authored-by: Jim Su <jimsu@protonmail.com>
This has been a headache for a long time, and we had #1071 and #1100 with the hope to fix the inconsistent behaviour across linters and environments. However, we recently found out that
double-quote-string-fixer
plugin in pre-commit-hook has inconsistent behaviour on python 3.11 and 3.12. See discussion here. This is sad because while this plugin enforces single quote behaviour with 3.11, it doesn't always enforce so with 3.12. Specifically, with f"str" syntax, this plugin allows both single quotes and double quotes with python 3.12.The problem is, some developers have black linter installed/integrated with their IDE, which is probably the most popular linter in python world (ranked by GitHub stars). This linter insists on always using double quotes. Now we have
black
anddouble-quote-string-fixer
fight each other (iff the developer uses python 3.12) for some quotes (f"str" syntax).After a lot of research, I couldn't find a way to enforce single quote behaviour without introducing a new dependency, flake8, together with a plugin for it to enforce quotes' behavior. The benefit of flake8 is it allows you to either enforce single quotes or double quotes, while black only allows you to enforce double quotes.
To avoid disruptions to ongoing PRs and tons of merge conflicts, this PR continues to enforce single quote behavior by using
flake8
rather thandouble-quote-string-fixer
. In addition, this PR adds a black config to pyproject.toml so that anyone using black formatter in their IDE now wouldn't attempt to change single quotes back to double quotes. Even if they do so, CI would reject it, but it's better for it not to happen at the first place.