-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add autoflake and isort to CI #12
Conversation
WalkthroughThe update introduces a new job in the GitHub Actions workflow specifically designed to enhance code quality by using Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
poetry.lock
is excluded by:!**/*.lock
pyproject.toml
is excluded by:!**/*.toml
Files selected for processing (2)
- .github/workflows/python_ci.yaml (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (1 hunks)
Additional comments: 1
prediction_market_agent_tooling/deploy/agent.py (1)
- 8-13: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-58]
The code in
prediction_market_agent_tooling/deploy/agent.py
appears to be logically structured and follows Python best practices. However, ensure that the removal ofAPIKeys
import does not affect any downstream code that might have been inadvertently dependent on it being imported here.
autoflake: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: ./.github/actions/python_prepare | ||
- name: Check with autoflake | ||
run: | | ||
poetry run autoflake --in-place --remove-all-unused-imports --remove-unused-variables --recursive . | ||
git diff --exit-code --quiet || exit 1 |
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.
The autoflake
job is correctly configured to remove unused imports and variables. However, consider adding a step to install autoflake
if it's not included in the .github/actions/python_prepare
action, as this could lead to a failure in environments where autoflake
is not pre-installed.
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.
Why not just git diff --exit-code
? 🤔
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.
@kongzii, you're right that git diff --exit-code
alone would suffice to check for differences, as it exits with 1 if there are differences and 0 otherwise. The --quiet
option is used to suppress the output, making the CI logs cleaner by not showing the diff output. The || exit 1
part, while seemingly redundant because git diff --exit-code
already exits with 1 on differences, is often included for explicitness in CI scripts. It ensures that the script exits with a failure status if differences are found, which is a common practice to make the failure conditions in CI pipelines more explicit. However, in this specific case, it's indeed redundant and could be omitted for simplicity.
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.
Thanks for the answer coderrabbit 🤣
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
poetry.lock
is excluded by:!**/*.lock
pyproject.toml
is excluded by:!**/*.toml
Files selected for processing (20)
- .github/workflows/python_ci.yaml (1 hunks)
- examples/cloud_deployment/gcp/agent.py (1 hunks)
- examples/cloud_deployment/gcp/deploy.py (2 hunks)
- examples/monitor/monitor.py (1 hunks)
- prediction_market_agent_tooling/config.py (1 hunks)
- prediction_market_agent_tooling/deploy/agent.py (1 hunks)
- prediction_market_agent_tooling/deploy/gcp/deploy.py (2 hunks)
- prediction_market_agent_tooling/deploy/gcp/utils.py (1 hunks)
- prediction_market_agent_tooling/gtypes.py (1 hunks)
- prediction_market_agent_tooling/markets/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/manifold.py (1 hunks)
- prediction_market_agent_tooling/markets/markets.py (1 hunks)
- prediction_market_agent_tooling/markets/omen.py (1 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (1 hunks)
- prediction_market_agent_tooling/tools/utils.py (1 hunks)
- prediction_market_agent_tooling/tools/web3_utils.py (1 hunks)
- scripts/bet_omen.py (1 hunks)
- tests/deploy/test_deploy.py (1 hunks)
- tests/markets/test_manifold.py (1 hunks)
- tests/markets/test_omen.py (1 hunks)
Files skipped from review due to trivial changes (3)
- examples/monitor/monitor.py
- prediction_market_agent_tooling/gtypes.py
- tests/markets/test_omen.py
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/python_ci.yaml
- prediction_market_agent_tooling/deploy/agent.py
Additional comments: 22
tests/markets/test_manifold.py (1)
- 2-5: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
examples/cloud_deployment/gcp/agent.py (1)
- 3-7: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
tests/deploy/test_deploy.py (1)
- 3-3: Consolidating imports from the same module into a single line improves readability and maintainability. This change aligns with best practices for organizing imports.
prediction_market_agent_tooling/config.py (1)
- 2-5: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
examples/cloud_deployment/gcp/deploy.py (1)
- 3-3: The addition of the
APIKeys
import fromprediction_market_agent_tooling.config
is necessary for accessing API key configurations within this file. This change is correct and aligns with the functionality requirements.prediction_market_agent_tooling/monitor/monitor.py (1)
- 4-7: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
scripts/bet_omen.py (1)
- 3-9: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. Additionally, removing the unused import of
HexStr
frometh_typing
is a good practice to keep the code clean and maintainable.prediction_market_agent_tooling/markets/markets.py (1)
- 2-11: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
prediction_market_agent_tooling/deploy/gcp/utils.py (1)
- 3-3: The addition of the
FunctionServiceClient
import fromgoogle.cloud.functions_v2.services.function_service.client
is necessary for interacting with Google Cloud Functions within this file. This change is correct and aligns with the functionality requirements.prediction_market_agent_tooling/deploy/gcp/deploy.py (2)
- 8-8: The addition of the
CronValidator
import is necessary for validating cron schedules within this file. This change is correct and enhances the functionality by ensuring the provided cron schedules are valid.- 6-9: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
prediction_market_agent_tooling/tools/web3_utils.py (1)
- 3-15: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
prediction_market_agent_tooling/markets/manifold.py (3)
- 3-7: The addition of the
requests
import is necessary for making HTTP requests within this file. This change is correct and aligns with the functionality requirements.- 5-5: The addition of the
APIKeys
import fromprediction_market_agent_tooling.config
is necessary for accessing API key configurations within this file. This change is correct and aligns with the functionality requirements.- 1-16: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
prediction_market_agent_tooling/markets/data_models.py (2)
- 1-1: The addition of the
typing as t
import is necessary for using typing annotations within this file. This change is correct and enhances the type safety and readability of the code.- 1-5: The import order has been adjusted to comply with PEP 8 guidelines, which recommend importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. This change improves readability and maintainability.
prediction_market_agent_tooling/markets/omen.py (5)
- 6-38: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-35]
The reordering of imports and grouping of related imports from the same package (
web3.types
andprediction_market_agent_tooling.*
) improves readability and adheres to PEP 8 guidelines. This change is approved.
- 36-38: The rearrangement of global variables
OMEN_TRUE_OUTCOME
andOMEN_FALSE_OUTCOME
immediately after the import statements is logical, making them easily identifiable. This change enhances code organization.- 6-38: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [39-58]
Loading ABI files using
with open(...) as f:
blocks immediately after global variables ensures that these critical resources are loaded at the start, which is a good practice for clarity and maintainability. Ensure that the paths to ABI files are correct and that these files are available in the specified directory.
- 6-38: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [59-67]
The placement of GraphQL query strings
_QUERY_GET_SINGLE_FIXED_PRODUCT_MARKET_MAKER
and_QUERY_GET_FIXED_PRODUCT_MARKETS_MAKERS
after ABI loading but before function definitions is appropriate, as it groups these constants near their usage context. This enhances readability.
- 6-38: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [68-261]
Function definitions appear to be unchanged in terms of their internal logic. The reordering of functions should not impact the functionality, assuming there are no dependencies on the order of definition due to Python's handling of function declarations. However, it's important to ensure that any changes in the order of function definitions do not inadvertently affect the program's behavior, especially in a dynamic language like Python where order can sometimes matter.
Lint on save working for me with this in my vscode settings.json
in combination with the black, isort and autoflake vscode extensions. For some reason I couldn't get the same black formatting behaviour with |
autoflake: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- uses: ./.github/actions/python_prepare | ||
- name: Check with autoflake | ||
run: | | ||
poetry run autoflake --in-place --remove-all-unused-imports --remove-unused-variables --recursive . | ||
git diff --exit-code --quiet || exit 1 |
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.
Why not just git diff --exit-code
? 🤔
Cool, will you add it to PMA too? |
Sure! |
Add isort and autoflake to CI, and run them on the codebase so CI passes.