Skip to content

Fix some non-critical pre-commit issues#41749

Merged
potiuk merged 1 commit intoapache:mainfrom
astronomer:fix-simple-pre-commit-issues
Aug 27, 2024
Merged

Fix some non-critical pre-commit issues#41749
potiuk merged 1 commit intoapache:mainfrom
astronomer:fix-simple-pre-commit-issues

Conversation

@uranusjr
Copy link
Member

I was running pre-commit run -a to fix some conflicts on database migration and found some interesting pre-commit failures that only occur in edge cases. Nothing serious, let’s just fix them. I don’t think any of these are common enough to require new checks.

The '.venv' name (common for in-project virtual environments) is not included in rat-excludes, causing a lint failure since a license is not found in it. Since 'venv' (no leading dot) is excluded, let's also add '.venv' to the file.

The 'downgradedb' method of BaseDBManager is declared abstract, but not implemented in FABDBManager. This causes Mypy to error when FABDBManager is instantiated. We don't always need the implement the method, so let's remove the abstractmethod decorator.

Finally (and least critically), when you run pre-commit during a merge, a hook checks if you're committing unresolved conflicts. The check is not particularly smart, however, and can mis-identify header underlines in restructuredText as false positives. This is easy to fix; just add one more character to the underline. It's fine as long as the underline is not EXACTLY SEVEN characters.

The '.venv' name (common for in-project virtual environments) is not
included in rat-excludes, causing a lint failure since a license is not
found in it. Since 'venv' (no leading dot) is excluded, let's also add
'.venv' to the file.

The 'downgradedb' method of BaseDBManager is declared abstract, but not
implemented in FABDBManager. This causes Mypy to error when FABDBManager
is instantiated. We don't always need the implement the method, so let's
remove the abstractmethod decorator.

Finally (and least critically), when you run pre-commit during a merge,
a hook checks if you're committing unresolved conflicts. The check is
not particularly smart, however, and can mis-identify header underlines
in restructuredText as false positives. This is easy to fix; just add
one more character to the underline. It's fine as long as the underline
is not EXACTLY SEVEN characters.
@potiuk potiuk merged commit 82abd79 into apache:main Aug 27, 2024
@uranusjr uranusjr deleted the fix-simple-pre-commit-issues branch August 28, 2024 06:58
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