Checks: Flake8 W605 - invalid escape sequence

[mshukuno]

The W605 issues in some files were not present. The rest of the files with W605 were regex expressions. To treat the string as raw, I added "r" before it.

[wenzeslaus]

This is mixed with the try-except changes. Can you please clean this up and I can review it then? It is much easier to review when number of files is low.

[mshukuno]

The reason for this seems to be that I created a branch while another branch is still being reviewed. My attempt to avoid the mistake I made at the beginning failed terribly. I should have waited for this PR so I wouldn't have this issue again.

I am having trouble fixing this. My goal was to delete all E722 updates with history from this branch. I couldn't get the desired result with revert and interactive rebase. Can this be restarted cleanly? May be I should have used reset and redo the W605 fixes instead of using revert. I would prefer to delete this pull request if it helps clean up this branch. If there is anything else I can try besides deleting this PR, please let me know.

[echoix]

It seems you created the branch from the branch of the other PR instead of starting it from main (your main, that would need to be updated from this repo). Same thing would have happened if you created a branch from your main, but your main has commits ahead this repo.

[echoix]

If this is a separate branch from your other PR, you can try again interactive rebase and drop the unrelated commits. You need to rebase using one commit before the one you want to act on. So here

git fetch
git pull
git rebase -i 50da15eb4ae33f68dcec3ecba1d544bf877fb919

Drop all commits that aren't for this PR. I think it's only the last two ones here, right?

Then, you need to force push, since you rebased.

git push --force --force-with-lease --force-if-includes

Then here we can rebase from main for the three commits behind

[echoix]

Is it ok like this? @mshukuno

[mshukuno]

Thank you @echoix! The fixes have just been pushed. Does this look right to you? It didn't occur to me that I had created a branch from another branch😩

[echoix]

I just rebased from here, so you might need a special pull on your side after this if there are some changes to do.

[echoix]

It looks right, but I don't think the squashed commit should have mentioned the rebase, as it will stay in the history, but won't make any sense. You can still reword it and force push before we start reviewing.

Once we start having comments, it's not a good idea to force push and have different commits, as we can't easily follow what changed, and what we already said.

Also, keep in mind that at the end, we use squash merge for PRs, so all commits end up in one, and the commit message is the list of the commits, unless we manually change the contents of that squashed commit message

[echoix]

I think you needed to use git pull --rebase when on GitHub it got rebased.

Would you mind trying to clean up again the three similar commits into 1?

(The problem was when pulling, it tried to keep both sides of the rebase)

This commit 266a58d has the CodeQL changes from main

[mshukuno]

It looks right, but I don't think the squashed commit should have mentioned the rebase, as it will stay in the history, but won't make any sense. You can still reword it and force push before we start reviewing.

Once we start having comments, it's not a good idea to force push and have different commits, as we can't easily follow what changed, and what we already said.

Also, keep in mind that at the end, we use squash merge for PRs, so all commits end up in one, and the commit message is the list of the commits, unless we manually change the contents of that squashed commit message

The previous commit message has been amended. Thanks for the Git collaboration workflow tips. I was wondering about CR processes, and you answered my question.

I have another question. During review of pull requests, can I rebase from upstream, or is there a timing for rebasing?

[echoix]

As I said, if there aren't any reviews (or review comments) yet, rebasing doesn't matter. It's after that that force pushing makes the existing comments only appear on commits that aren't part of the PR anymore.

And when there is a discussion, and the PR's author makes changes after that, if a rebase has happened, it's really hard to follow what changed, and we need to start again from scratch instead of simply continuing from where we left, and knowing that we already looked at the rest.

Also, as CI is run after ever PR pushes, and there is a limit of concurrent CI runners (20) shared for all projects in the OSGeo organization. We try to be nice for other PRs here and other projects. So it's better to double check the PR's content in the preview before submitting. It would show if the PR has conflicts, which commits will be included etc.

Nothing prevents you from trying a future PR by making a PR against your fork (instead of OSGeo/grass), and when ready, file a PR here from the same branch. You just won't have the yellow popup box to help you, you'd need to go on the grass repo, in the PRs, then the green button for a new PR, and find your branch. The CI can run on your side as you wish in a PR to your fork before the one here.

[echoix]

Rereading your last question, I think I understood it differently. If you mean keeping it up to date, it is not necessary unless it is getting old, and some merged changes could have an impact on the validity of the CI results.
Things like other changes in the same modules that this PR concerns, major changes in the CI, etc.

As an opposite extreme example, if every PR stays updated with the main branch at every commit, that would mean that every PR merged would need to have the CI ran again for all the 100 other open PRs. We see that it isn't really useful. If the PR is old and finally ready to merge, but we have a doubt that the CI might fail once merged in main, we (from grass) might merge main into the PR branch just before merging, and if it still passes, then we'd merge.

Does this or the other answer your question?

[echoix]

Great! now it's ready to review ;)

[wenzeslaus]

While simply adding r is safe for one backslash, cases when backslashes may mean actual escape character need more care.

Simply adding strings which are producing the same result without examining the context seems to be the way to go here.

[wenzeslaus]

...unless we manually change the contents of that squashed commit message

I hope we are doing that when needed :-)

[mshukuno]

One file has been modified. The rest of the files have a "r" before the strings. Does this sound good to you? The code will be pushed if that is the case. Please let me know if there are any preferred comments for CR fixes.

[wenzeslaus]

The code will be pushed if that is the case.

For small changes and already written code, please push before approval. Then we can react to it right away.

Please let me know if there are any preferred comments for CR fixes.

Sorry, I don't know what CR refers to, but no comments in the code are needed for the fixes here. Describing the change in the commit message will suffice. It is unfortunate that comments are missing in the original code, but we would have to spend much more time examining that.

[wenzeslaus]

Great, thanks!

[echoix]

Thanks for keeping up and having another contribution merged!