Conversation
updated import order, applied isort to adjust the formatting of the import packages for py files
to test if files pass
gabe-l-hart
left a comment
There was a problem hiding this comment.
Thanks for submitting this! This has been a gross corner for a while. I think we need to look carefully at the profile we're using for isort (some of the changes look wrong to me). Additionally, I'm concerned about possible conflicts between ruff and isort which can cause pre-commit to become inoperable (the issue I ran into that caused me to disable this originally).
cforge/commands/server/run.py
Outdated
| # Third Party | ||
| # First-Party |
There was a problem hiding this comment.
| # Third Party | |
| # First-Party | |
| # First-Party |
There was a problem hiding this comment.
We still need to remove the duplicate # Third Party heading
cforge/config.py
Outdated
| # Standard | ||
| from contextlib import contextmanager | ||
| from functools import lru_cache | ||
| import os |
There was a problem hiding this comment.
This looks wrong. isort should keep from ... imports separate from import ... imports.
There was a problem hiding this comment.
**ignore my previous comment, I see what you mean, import os is in the middle of the from imports; for some reason this does pass the isort --check-only command - will take a look
There was a problem hiding this comment.
Typically, I've used imports like the following:
from baz import bat
from foo import bar
# ^ alpha sort for all `from ...` imports
import asdf
import qwer
# ^ alpha sort for all `import ...` imports(but without the editorial comments in between)
There was a problem hiding this comment.
gotcha- i found the issue:
there was one style param force_sort_within_sections = true (sort the imports by module, independent of import style) which was overriding from_first= true which does the sorting you mentioned
I got rid of that, then added force_alphabetical_sort_within_sections = true to sort each by alphabetical order
tests/commands/resources/test_a2a.py
Outdated
| import json | ||
| import tempfile | ||
| from pathlib import Path | ||
| import tempfile |
There was a problem hiding this comment.
Same here, I think the isort settings must be a bit off from what I'm used to
cforge/profile_utils.py
Outdated
| from pydantic import BaseModel, Field, field_validator, ValidationInfo | ||
|
|
||
| # Local | ||
| # First-Party |
There was a problem hiding this comment.
I think we want cforge to be Local, not First-Party
There was a problem hiding this comment.
checked and cforge was set as a known first party in the isort config in the toml, I'll adjust it to known local package!
There was a problem hiding this comment.
Yeesh, this is what I get for copying all this config from the upstream project rather than starting from scratch!
pyproject.toml
Outdated
| # TODO: Enable "I" for import sorting as a separate PR. | ||
| select = ["E3", "E4", "E7", "E9", "F", "D1"] | ||
| # Also "D1" for docstring present checks and "I" for import sorting. | ||
| select = ["E3", "E4", "E7", "E9", "F", "D1", "I"] |
There was a problem hiding this comment.
I'm not clear whether it's this change or the one above to enable the pre-commit hook that actually caused all the changes. I'm also concerned that ruff's I might differ from isort w/ --profile=black.
Here's an example of the isort config that I'd like to use for this project: https://github.com/caikit/caikit/blob/main/.isort.cfg. I think there's a way to add this config directly in pyproject.toml without an independent .isort.cfg file which would be the preferred method.
There was a problem hiding this comment.
it was the change to enable the isort pre-commit hook that caused these changes. i do see ruff's I is causing some sort of issue in the pre-hook.. just tried it again and it fails the black python reformatter and the isort formatting; I'll re-disable it
I think earlier either I might have run the prehook and the "I" didn't save properly so I didn't see any problems, which is completely my bad
updated configs for isort and re-disabled the toml.
Co-authored-by: Gabe Goodhart <ghart@us.ibm.com>
gabe-l-hart
left a comment
There was a problem hiding this comment.
Two more small style change requests:
- Figure out how to remove the extra newline between the
fromandimportlines within a given section - If possible, use case-sensitive alphabetical sorting within sections
There are also a number of local imports in unit tests that show up because the headers for them are changing. This should all get removed, but that can happen in a different PR.
| from pathlib import Path | ||
| from typing import Optional | ||
|
|
||
| import json |
There was a problem hiding this comment.
There shouldn't be a newline before this. It looks like this change (a newline between the from and import sections) is true in all the files, so probably still some subtle setting to fix.
There was a problem hiding this comment.
removed lines_between_types = 1 in the config!
cforge/commands/server/run.py
Outdated
| # Third Party | ||
| # First-Party |
There was a problem hiding this comment.
We still need to remove the duplicate # Third Party heading
|
|
||
| def handle_exception(exception: Exception) -> None: | ||
| """Handle an exception and print a friendly error message.""" | ||
| # Local |
There was a problem hiding this comment.
NOTE to self: Make sure this is a valid dynamic import to avoid a cycle
cforge/credential_store.py
Outdated
| from cryptography.hazmat.backends import default_backend | ||
| from cryptography.hazmat.primitives import hashes | ||
| from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes | ||
| from cryptography.hazmat.primitives.ciphers import algorithms, Cipher, modes |
There was a problem hiding this comment.
It looks like this is doing case-insensitive alphabetical sort. In other projects, I've used case-sensitive so all capitalized come first (ALL_CAPS -> CamelCaps -> lower). It's not a setting I have a strong preference about, but I think that's maybe more standard?
There was a problem hiding this comment.
sounds good, changed it to case sensitive (set case_sensitive = true in config)
pyproject.toml
Outdated
| ############################################################################### | ||
| known-first-party = ["cforge", "mcpgateway"] # treat "cforge" and "mcpgateway" as FIRSTPARTY | ||
| known-local-folder = ["tests", "scripts"] # treat these folders as LOCALFOLDER | ||
| known-first-party = ["mcpgateway"] # treat "cforge" and "mcpgateway" as FIRSTPARTY |
There was a problem hiding this comment.
| known-first-party = ["mcpgateway"] # treat "cforge" and "mcpgateway" as FIRSTPARTY | |
| known-first-party = ["mcpgateway"] # treat "mcpgateway" as FIRSTPARTY |
removed line between from and import types changed sorting to case sensitive
just removed the style changes that are no longer applied from the toml file
gabe-l-hart
left a comment
There was a problem hiding this comment.
One. More. Time! It's all ready to approve, but we need to pull out that spurious PR_ISORT.md file since that should not be committed to the repo.
PR_ISORT.md
Outdated
There was a problem hiding this comment.
This should be removed
Branch: isort-fix AI-usage: none Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>
|
woohoo! also i don't think I have write permissions haha so you'll have to merge it |
Enable isort for Import Sorting
Summary
Enables isort in pre-commit hooks and applies consistent import sorting across the codebase using the black-compatible profile.
Changes
.pre-commit-config.yaml: Enabled isort hook (v6.0.1) with--profile=blackpyproject.toml: Added "I" to Ruff lint rules, removed TODO commentImpact
Resolves TODO in
pyproject.tomlline 175.Address #24