Skip to content

chore: bump pre-commit versions, plus small fixes #1643

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

Merged
merged 8 commits into from
Mar 25, 2025

Conversation

tonyandrewmeyer
Copy link
Contributor

Possibly simplest to review commit by commit.

This bumps the versions of the pre-commit tools.

A few small issues are also fixed (this is somewhat a subset of #1474, skipping all the docs and controversial changes).

  • A few strings are formatted more cleanly (including where previous tooling has put in " " breaks).
  • A fix to a framework test (found in chore: run pre-commit on all files #1474).
  • A fix to a Harness test.
  • Removing shebangs in Scenario code (none of the files are executable).

Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

I went commit by commit. Looks good. The accidental 'bar--' 'FOO' concatenation was interesting in that the tests work either way, which is surprising at first. Left a suggestion for perhaps clearer test cases, but not a blocker.

@benhoyt
Copy link
Collaborator

benhoyt commented Mar 23, 2025

Happy with this. I'm not quite sure though -- why aren't these things caught in CI by the ruff checks there?

@james-garner-canonical
Copy link
Contributor

I think it's because this jumps to using the latest 0.11 ruff version, while we're using 0.7 in ci currently, which I should make a PR to bump. Merging the fixes here first should make that smoother, thanks, Tony!

@@ -361,7 +361,7 @@ def _on_cluster_relation_changed(self, event: ops.EventBase):
harness.set_leader(False)
harness.update_relation_data(rel_id, 'test-app', {'k': 'v3'})
assert backend.relation_get(rel_id, 'test-app', is_app=True) == {'k': 'v3'}
assert len(harness.charm.observed_events), 1
assert len(harness.charm.observed_events) == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

ouch!

@tonyandrewmeyer tonyandrewmeyer merged commit 427138a into canonical:main Mar 25, 2025
32 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the fix-pre-commit branch March 25, 2025 09:34
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.

4 participants