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

Use ruff --check because ruff suggests it #33

Merged
merged 2 commits into from
Jul 21, 2024

Conversation

hmaarrfk
Copy link
Contributor

No description provided.

@Vipitis
Copy link
Collaborator

Vipitis commented Jul 21, 2024

I think it needs to be ruff check . it took me a few commits to get right too

@hmaarrfk
Copy link
Contributor Author

of course.. but now it finds an error locally. hoping i have my local environment misconfigured to pass the CIs.

@Vipitis
Copy link
Collaborator

Vipitis commented Jul 21, 2024

I think there are some rule changes in ruff that require one or two changes, but I that is already included in #30 and completely unrelated to the original change your proposed. feels a bit like https://edw519.posthaven.com/it-takes-6-days-to-change-1-line-of-code

@hmaarrfk
Copy link
Contributor Author

@hmaarrfk
Copy link
Contributor Author

the reason i chose my strategy is that lists and tuples are often not guaranteed:

>>> [] == ()
False

@Vipitis
Copy link
Collaborator

Vipitis commented Jul 21, 2024

the reason i chose my strategy is that lists and tuples are often not guaranteed:

>>> [] == ()
False

json can't hold tuple, it mangles them to a list. But I have used that function with dicts before so it's a good solution.
the CI doesn't yet run the API tests (we don't have a key set as repository secret) - but I confirmed it to work as expected locally.

@Vipitis Vipitis self-requested a review July 21, 2024 18:58
@Vipitis Vipitis merged commit 9d37dab into pygfx:main Jul 21, 2024
11 checks passed
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