normalized CER

[bertsky]

But perhaps this should be in the form of a user choice / parameter, in which case of course the change would be much bigger.

[mikegerber]

Because I was complaining elsewhere: I welcome this PR, I just want to give it a proper discussion and review as it gives different CER values compared to the current implementation in master.

[mikegerber]

I've rebased this to the current master branch, to fix the tests.

Changing to draft for now, because I would probably want at least to make this optional. Please don't change anything just yet, I want to review it/the normalized CER first and this probably have to wait until after Easter.

[bertsky]

mikegerber force-pushed the normalized-cer branch from a33b713 to 7a79bae
5 hours ago

Speaking about Github etiquette and collaboration best practices: force-pushing on someone's forked PR is really bad practice!

You yourself have asked me to turn this into a PR. I was relying on that branch for my own (and other users') purposes. Now I have to go back in the reflog (if it's still there) to re-instate that branch on my own fork...

Changing to draft for now, because I would probably want at least to make this optional. Please don't change anything just yet, I want to review it/the normalized CER first and this probably have to wait until after Easter.

Sure, if you want to make that optional, and give it proper documentation, the change would have to be bigger.

(For me it was just a matter of necessity.)

[mikegerber]

Speaking about Github etiquette and collaboration best practices: force-pushing on someone's forked PR is really bad practice!

No it isn't? It's the way a maintainer can make changes to a PR. And this case it was just a rebase to let the tests run.

You yourself have asked me to turn this into a PR. I was relying on that branch for my own (and other users') purposes. Now I have to go back in the reflog (if it's still there) to re-instate that branch on my own fork...

Ah, I wasn't aware of that. Need to think about this. It certainly wasn't my intention to destroy anything and we may need to establish a workflow that makes both uses possible (1. you keep your branch and 2. I can make changes to the PR)

[mikegerber]

No it isn't? It's the way a maintainer can make changes to a PR. And this case it was just a rebase to let the tests run.

Was the rebase (with the force-push) the issue and not the editing? I think I can work with that, either by

A. maybe cherry-picking the master fixes would have sufficed
B. coordinating the rebase before I do it

[mikegerber]

The current changes illustrate a part of the problem: because you use the same branch for your fork and the PR I now have unrelated changes in the PR (which are already fixed in master).

If you don't use a separate branch for the PR, all I am left with is closing this, and to create a new branch for a new PR.

[mikegerber]

All things considered, the issue seems to be that you use your fork as the source branch for the PR. If you would create a new branch for the PR (one command!), the problems go away.

If you would just create a new branch (e.g. pr-foo-bar) identical to the original branch (e.g. my-fork), this would

a. enable the maintainer to work on pr-foo-bar

b. not break your my-fork

Maintainers working with the source branch of the PR also is the way for a maintainer to make changes to the PR. GitHub even has the option to disabe it. The only other possibility for a maintainer to change a PR is to close it and make a new PR.