Skip to content

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Feb 5, 2024

Description of proposed changes

Fixes the following ruff violations with Python 3.10+

Addresses #3039 (comment)

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.
  • Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash command is:

  • /format: automatically format and lint the code

@weiji14 weiji14 added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Feb 5, 2024
@weiji14 weiji14 self-assigned this Feb 5, 2024
@weiji14 weiji14 changed the title ruff: Fix ruff: Fix Python 3.10 violations Feb 5, 2024
@seisman seisman added this to the 0.12.0 milestone Feb 5, 2024
@weiji14
Copy link
Member Author

weiji14 commented Feb 6, 2024

[ ] UP038 Use X | Y in isinstance call instead of (X, Y) - https://docs.astral.sh/ruff/rules/non-pep604-isinstance (should we ignore this, because the docs says X | Y might be slower than (X, Y)?)

Do we want to ignore UP038, or change to isinstance(..., X | Y)? I can run some benchmarks to see if it's noticeably slower. Edit: Benchmarks at https://codspeed.io/GenericMappingTools/pygmt/branches/ruff/python-3.10

@weiji14 weiji14 added the run/benchmark Trigger the benchmark workflow in PRs label Feb 6, 2024
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good.

@weiji14
Copy link
Member Author

weiji14 commented Feb 6, 2024

Wanted to double-check the flame graphs, so I manually triggered the benchmarks workflow on the main branch to get the baseline (following steps at #2910 (comment)). Will merge this after I've had a look at the timing comparison.

@weiji14 weiji14 added run/benchmark Trigger the benchmark workflow in PRs and removed run/benchmark Trigger the benchmark workflow in PRs labels Feb 6, 2024
@weiji14
Copy link
Member Author

weiji14 commented Feb 6, 2024

Hmm, still not seeing the flame graph, but the performance benchmark didn't report the run as noticeably slower https://github.com/GenericMappingTools/pygmt/pull/3040/checks?check_run_id=21254596529, so should be ok.

@weiji14 weiji14 marked this pull request as ready for review February 6, 2024 06:20
@weiji14 weiji14 merged commit 31688c4 into main Feb 6, 2024
@weiji14 weiji14 deleted the ruff/python-3.10 branch February 6, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs run/benchmark Trigger the benchmark workflow in PRs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants