Skip to content

refactor: Enable style checks in CI and fix styling#82

Open
amcintosh wants to merge 3 commits intoTheDevOpsBlueprint:mainfrom
amcintosh:refactor/enable-flake8-black
Open

refactor: Enable style checks in CI and fix styling#82
amcintosh wants to merge 3 commits intoTheDevOpsBlueprint:mainfrom
amcintosh:refactor/enable-flake8-black

Conversation

@amcintosh
Copy link
Contributor

@amcintosh amcintosh commented Oct 11, 2025

PR Checklist

  • Follows single-purpose principle
  • Tests pass locally (if applicable)
  • Documentation updated (if needed)

What does this PR do?

The original work to add the CI left the flak8 and black checks passing on failures. This was due to the number of style fixes that needed to be addressed in a future PR (see #40)

This work:

  • Updates the CI action to fail on flake8 and black violations.
  • Moves to the "Flake8-pyproject" library as the regular flake8 library does not read config from pyproject.toml (just .flake8 or setup.cfg)
  • Runs black against all files in alix/ and tests/
  • Runs flake8 against all files and corrects any violations

Apologies for the large PR. It seemed best to do it all in one shot. The changes are mostly whitespace, import reorganization, and removing f-strings when not required. There were some method redefinition violations in cli.py that required renaming, but the cli commands are unchanged. If you would prefer I can break this up into smaller fixes.

Related Issue

Fixes #70

Type of change

  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Configuration change
  • Documentation update
  • Setup/Infrastructure

@skalwaghe-56
Copy link
Contributor

I haven't checked all the files as its mostly refactoring so looks good for me.

@Valentin-v-Todorov
Copy link
Contributor

Valentin-v-Todorov commented Oct 12, 2025

Hey @amcintosh @skalwaghe-56 ! Thanks for tackling the linting issues.

The work looks solid overall - moving to Flake8-pyproject and running black/flake8 across the codebase is exactly what was needed.

However, I found a critical bug in alix/tui.py that needs fixing before we can merge. You have two update_status methods defined (around lines 738-752 and then again later).

The second one has debug text with "Showing2" and "Total2" which suggests it's leftover from testing.

Python will silently overwrite the first method with the second, which could break the status bar functionality. Please remove the duplicate and keep whichever version is correct.

Also, the change from bare except: to except Exception: in tui.py line 825 is good, but double-check that this won't change error handling behavior anywhere critical.

Once you fix the duplicate method issue and verify the exception handling changes work correctly, this should be good to merge!

@skalwaghe-56
Copy link
Contributor

Hey @amcintosh @skalwaghe-56 ! Thanks for tackling the linting issues.

The work looks solid overall - moving to Flake8-pyproject and running black/flake8 across the codebase is exactly what was needed.

However, I found a critical bug in alix/tui.py that needs fixing before we can merge. You have two update_status methods defined (around lines 738-752 and then again later).

The second one has debug text with "Showing2" and "Total2" which suggests it's leftover from testing.

Python will silently overwrite the first method with the second, which could break the status bar functionality. Please remove the duplicate and keep whichever version is correct.

Also, the change from bare except: to except Exception: in tui.py line 825 is good, but double-check that this won't change error handling behavior anywhere critical.

Once you fix the duplicate method issue and verify the exception handling changes work correctly, this should be good to merge!

Nice catch! I wan't able to find this because I didn't check everything (will need to keep that in mind next time!)

@amcintosh amcintosh force-pushed the refactor/enable-flake8-black branch from 9941dc6 to a2f3919 Compare October 12, 2025 15:23
@amcintosh
Copy link
Contributor Author

@Valentin-v-Todorov I've fixed the redefinition issue. I had removed the second occurrence but I should have removed the first with the leftover debug messages.

I've also rebased from main and incorporated the latest merges.

As for the exception change, I ran tui and raised an exception where clipboard.copy(alias_cmd) is called and it behaved as expected.

@skalwaghe-56
Copy link
Contributor

@amcintosh Please resolve the conflicts.

The original work to add the CI left the flak8 and black checks passing
on failures. This was due to the number of style fixes that needed to be
addressed in a future PR.

This work:

- Updates the CI action to fail on flake8 and black violations.
- Moves to the "Flake8-pyproject" library as the regular flake8 library
  does not read config from pyproject.toml (just .flake8 or setup.cfg)
- Runs black against all files in alix/ and tests/
- Runs flake8 against all files and corrects any violations
Running flake8 detected two definitions of `update_status` in `tui.py`.
I initially removed the second, but since it redefined the method, I
should have removed the first (which appeared to have debug messaging).
This fixes that.
@amcintosh amcintosh force-pushed the refactor/enable-flake8-black branch from 3af657e to 9488497 Compare October 17, 2025 00:17
@amcintosh
Copy link
Contributor Author

@Valentin-v-Todorov, @skalwaghe-56 I've rebased with the latest changes from main. Should be good to go again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decide Action for Some CI steps

3 participants