Skip to content

Conversation

@dmohns
Copy link
Member

@dmohns dmohns commented Oct 16, 2023

Apply'ing black and ruff to the repository.

Doing this in a first step, such that we can add the merge commit to .git-blame-ignore-revs file in a second step, see https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

After that, adding CI to check linting of Pull Requests.

Note: This should be merged using Squash&Rebase

Copy link
Member

@benmod benmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good except those imports in the init.py file that the linter wrongly thought were unused but are actually used by anybody using the library.

@@ -1,9 +1,5 @@
from .token_encode import OpenPAYGOTokenEncoder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing those imports? They are left at top level so that you can access them importing just the module, if we remove them it will break existing tools using it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It raised "unused variable" warnings. I think the recommended approach is to use __all__. Let me try to adapt. Do you have a piece of code, where I can test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing public but basically you should be able to import all of those variables when importing openpaygo. Like from openpaygo import TokenType should work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally find it a bit odd, if from openpaygo import TokenType works, but from openpaygo import * and then openpaygo.TokenType doesn't. Using __all__ should be non-breaking. WDYT?

Copy link
Member

@benmod benmod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good now!

@dmohns dmohns merged commit a8f0227 into main Nov 2, 2023
@dmohns dmohns deleted the add-linters-and-auto-formatter branch November 2, 2023 15:39
@dmohns dmohns mentioned this pull request Nov 2, 2023
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.

3 participants