-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
temporal: Fix FLY002: String-join inefficiencies in temporal/t.register tests #5087
base: main
Are you sure you want to change the base?
Conversation
@arohanajit Can your titles when making fixes using ruff be more specific? If you're fixing one rule, for one module, it will be way easier to find and understand when looking at the history what was changed. There's hundreds of rules from all linters in ruff, and hundreds of files here, so I feel like saying "ruff errors" could be applied to dozens and dozens of PRs doing totally different things. But, at least, you add the error code in the PR, but most often, since your commit messages only mention "update", once merged, that error code isn't searchable, when searching for a commit. |
To know if it's specific enough: put yourself as the one that needs to find a commit to revert since something broke. You know what rule change caused the problem, and try to find the commits up to where do your bisect. If it's hard to find these commits, it's maybe not precise enough |
To add to what @echoix is saying, it is clear that you are trying to fix things (including the word "Fix" is great, but not enough). It is also clear which directory or file it is when just a prefix or one name is included (t.register or temporal, but not both). Similarly, it does not add much to know that you are fixing errors, that's sort of expected, but which errors, that's the question. If it is hard to express all changes in a title and be specific enough at the same time, maybe it is time to split the PR into two. To @echoix's note on trying to see where the commit message (PR title) appears see (latest commits, history of one file, and blame): https://github.com/OSGeo/grass/commits/main/ |
To be clear, the work is not bad ;) |
Thanks @wenzeslaus and @echoix I'll keep this in mind in future PRs |
So are you able to edit the PR title or you don't have permissions? |
"prec_6|...|2499-11-16 21:00:00|None", | ||
] | ||
) | ||
ref_str = """name|mapset|start_time|end_time... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is still failing in the CI
@arohanajit unrelated to this PR, if you check the CI, there is lot's of noisy numpy warnings and ResourceWarnings, could you look into that (not in this PR)? I wonder if there was any issue/discussion about this already, do you know @echoix? |
The numpy warnings are from doctests, and that is what is blocking testing with newer OS/Python versions like 3.13 First issue: Solving this would unblock
Also see:
After all that time, I feel like we might need to decide to not run doctests for every version. It would be impossible to always satisfy all. (Unless the doctests have conditionnals, that would be ugly) |
For the resource warnings, at build time I see that there's some related to the files that make documentation (maybe mkhtml), so probably a fix like #5063 needs to be applied, or the devnull one from #4917 (comment) |
Otherwise, it might be in doctests that we'll need to look at |
Got it, I'll start working on this after finishing with all the ruff errros |
Fixed
FLY002
and usedf-strings
instead ofjoin
int.register/