Conversation
There was a problem hiding this comment.
I suspect that committing a HERE-removal plugin is not needed. We should just remove the HERE definition instead (after removing all HEREs, of course) -- the compiler will do the rest for us. A stand-alone script is still required (so that folks can apply it to other branches), but it should not be officially committed to master because it is not needed for the master branch and future official code. This PR can focus on the manual HERE removal, for example.
Please remove changes introduced by the HERE-removal script from the PR so that it is possible to review the substance of this PR without finding the right files among the 192 files. Just post a link to the commit removing HEREs instead. That commit does not have to be on this PR branch.
Please remove non-HERE related changes from this PR. Edit: If there are just a few, I can ignore them until #143 is closed.
|
The substance of this PR is only the contents of the added scripts/maintenance/deprecated-wrappers file. Everything else is automated by running ./scripts/source-maintenance.sh with that file staged. |
FWIW, I am not aware of any disagreement regarding that statement. |
|
Huh? so why do you want the automated diff removed prior to review? It's there so if you choose you can look through the whole set of side effects that will occur - otherwise you can review just that script itself. If you you meant you are okay with this script being merged please say so clearly and I will rebase without the auto-changes, and mark for Anubis. |
IIRC, I wanted that for two reasons: To avoid waiting for you to tell me which file(s) were manually modified and to be able to quickly check that your definition of substance matches the actual PR changes. As you know, it is not uncommon for PR authors to sneak in (accidentally or on purpose) undisclosed out-of-scope changes, especially in huge PRs. As you probably know, I have suggested how to deal with large-scale automated changes at #143 (review) . That sketch applies to this PR as well AFAICT. Please note that cleaning up the diff for the initial review is a secondary concern. The necessity of committing the HERE-removal plugin (i.e. the first paragraph in my review) is the primary concern for this PR. I recommend focusing on that. |
FYI: I suspect that GitHub does not create merge commits for draft pull requests, leading to CI failures like the Semaphore CI check_diff error quoted above. I could not quickly find any firm official confirmation about the lack of merge commits for draft PRs, but this limitation feels natural to me. If that is indeed what is going on, then we may want to add CI code to treat draft PRs specially. |
Prunes away over 90% of allHERE macro uses.
yadij
force-pushed
the
cleanup-here-removals
branch
from
364b224 to
495e2d7
Compare
|
Very well. Rebased to contain just the cleanup rules. |
2 participants
Draft rules to drop over 90% of HERE macros automatically.
The remaining HERE need manual attention to replace with
useful debug texts.
This PR is scope is the maintenance script rules. As a transitional
update to reduce the removal pains. eg old patches containing
HERE can still be tested during the transition period and even
(if QA allows) merged.