Skip to content
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

Enforce formatting #95

Merged
merged 3 commits into from
Mar 23, 2024
Merged

Conversation

ranebrown
Copy link
Contributor

Adds a job to check formatting and formats all existing files with ruff.

Config options if you want anything changed from the defaults.

self.build_account_map()

self.initialized = True

def build_account_map(self):
# map transaction types to target posting accounts
self.target_account_map = {
"buymf": self.config['cash_account'],
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to have these be left untouched? It's much more readable the way it currently is IMHO. There are several of this type of block.

@@ -34,6 +34,4 @@ def get_balance_statement(self, file=None):

date = self.get_balance_assertion_date()
if date:
yield banking.Balance(
Copy link
Owner

Choose a reason for hiding this comment

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

Could these types of cases be excluded? Perhaps by increasing line width? IMHO, the formatted version is less readable here.

@redstreet
Copy link
Owner

Hello there, thanks for the PR! I think this is in general a good idea, and in particular helpful to keep contributions from multiple users consistent. However, it does seem to reduce readability in a few cases. I quickly highlighted a couple patterns, there are probably a few others. If you're familiar with ruff and its options, it'd be great if you could help figure out those cases at least, and I'd be happy to take it in. Thanks again!

@ranebrown
Copy link
Contributor Author

I did a bit more digging and the option set is pretty limited. The ruff formatter pretty much matches black which intentionally gives you very few options to change so all code formatted with it should look the same. For specific cases this can be done:

# fmt: off
code_here()
...
#fmt: on

I would argue consistency across the board is better but let me know if you want formatting disabled on the sections you pointed out.

@redstreet
Copy link
Owner

Thanks for looking this up. You make a good point, and I agree overall. I still feel like target_accounts_map should be the one exception and remain unformatted, but I'm fine with leaving it to ruff on everything else for the sake of consistency.

f you're okay making the change to exclude those blocks alone, I'd be happy to take it in. Thank you!

@ranebrown ranebrown force-pushed the Enforce_Formatting branch from facce82 to 142c5f6 Compare March 23, 2024 20:56
@ranebrown
Copy link
Contributor Author

Made a few more changes:

  1. Added exceptions for transaction_type_map and header_map.
  2. Changed max line length to 88 which makes things more readable in most cases (easy to change back to 127 if you prefer).
  3. Added isort formatting
  4. Moved ruff (and isort) config to a pyproject.toml file

@redstreet redstreet merged commit b5f2855 into redstreet:main Mar 23, 2024
1 check passed
@redstreet
Copy link
Owner

redstreet commented Mar 23, 2024

Looks great, I appreciate the PR and the changes! This was long needed. Merged. Thank you!

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.

2 participants