-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
lint: simplify hooks already covered by Ruff #1204
Conversation
Agree with you, I guess it was introduced here #1112 to prevent the single-quote issue related to different python versions, but I guess we can abandon flake8 and autopep8, also can you try to remove the deps from pyproject.toml file |
done :) |
CC @li-boxuan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to wait for @li-boxuan to take a look at this since he worked through some tricky linting issues w/ flake8 recently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate your attempt to reduce the complexity of the pre-commit hooks (which I would love to do too), I am pretty sure flake8 is not covered by Ruff. Autopep8 might be, I need to check.
To prove that flake8 is not covered by Ruff, you could find anything like
f'some string'
in the codebase, change it to double quotes, and run make lint
. You will see flake8 would complain it shall be single quotes.
Now if you remove flake8 (as what you did in this PR), you would see no complaint. Ruff would approve the change. Now we would start the 'single-quote' v.s. 'double-quote' war again because some developers' linters prefer single quotes and others prefer double quotes.
ChatGPT tells me AutoPep8 is not covered by Ruff: https://chat.openai.com/share/7214f2e7-853c-46dd-844a-5de6af6934da |
@Borda |
then it shall be reposted as bug :) |
just tested, and by enabling the basic rules like E, W, and F to have parity with flake8, the issues is recognized correctly as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just cloned your branch, and applied the following patch:
diff --git a/agenthub/monologue_agent/utils/monologue.py b/agenthub/monologue_agent/utils/monologue.py
index 8dfb4ce..88c414b 100644
--- a/agenthub/monologue_agent/utils/monologue.py
+++ b/agenthub/monologue_agent/utils/monologue.py
@@ -75,4 +75,4 @@ class Monologue:
self.thoughts = prompts.parse_summary_response(summary_resp)
except Exception as e:
traceback.print_exc()
- raise RuntimeError(f'Error condensing thoughts: {e}')
+ raise RuntimeError(f"Error condensing thoughts: {e}")
I was able to commit without issues. On main, however, pre-commit hook will fail the commit to enforce single quotes (which is what we want).
You probably want to incorporate the changes suggested by @dorbanianas.
@li-boxuan addressed your comments, mind rechecking it? 🦩 |
@Borda This is neat, but... I tried #1204 (review) again on your branch and still the same problem. Looks like ruff isn't able to enforce single quote behavior, or you did something wrong. |
Hmmm, let me double-check it and eventually report it as an issue... UPDATE: trying to reproduce, found the line in the codebase including pre-commit run --files opendevin/**/* agenthub/**/* --show-diff-on-failure --config ./dev_config/python/.pre-commit-config.yaml And seeing all checks pass without any change means that ruf-format is okay with it... @li-boxuan, could you pls share the steps to reproduce your case? |
@li-boxuan addressing it with another pre-commit hook - https://github.com/pre-commit/pre-commit-hooks/blob/main/README.md#double-quote-string-fixer UPDATED: seems to be fixed with Ruff linter and enabling "Q" rule - astral-sh/ruff#7834 |
This is what we removed earlier, because it doesn't reliably convert double quotes to single quotes when python 3.12 is used. Could you please test locally with python 3.12 installed? |
@li-boxuan sure, that is why I used the Q rule which found several more issues |
I generated a patch for testing: diff --git a/agenthub/codeact_agent/codeact_agent.py b/agenthub/codeact_agent/codeact_agent.py
index 9ff503f..74bdfe2 100644
--- a/agenthub/codeact_agent/codeact_agent.py
+++ b/agenthub/codeact_agent/codeact_agent.py
@@ -108,7 +108,7 @@ class CodeActAgent(Agent):
{'role': 'user', 'content': obs.content})
elif isinstance(obs, CmdOutputObservation):
content = 'OBSERVATION:\n' + obs.content
- content += f'\n[Command {obs.command_id} finished with exit code {obs.exit_code}]]'
+ content += f"\n[Command {obs.command_id} finished with exit code {obs.exit_code}]]"
self.messages.append({'role': 'user', 'content': content})
else:
raise NotImplementedError( Using python 3.12, I can confirm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you sorted it out eventually! I am happy to see fewer linters/plugins.
C.C. @rbren
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just came to my mind that you also need to change Makefile, specifically, the following snippet, since your PR decides to lint all files instead of just two folders:
lint:
@echo "$(YELLOW)Running linters...$(RESET)"
@poetry run pre-commit run --files opendevin/**/* agenthub/**/* --show-diff-on-failure --config $(PRECOMMIT_CONFIG_PATH)
@li-boxuan updated in 068d7cd |
Head branch was pushed to by a user without write access
@rbren, what is the best way to resolve the Poetry lock issue / update it with minimal changes... |
@rbren mind approve the CI ru, pls 🐿️ |
flake8
andautopep8
are already covered by Ruff with--fix
argument