Skip to content
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

Only remove kw-replacement overlays in adoc-unfontify-region-function #46

Merged
merged 8 commits into from
Feb 1, 2024

Conversation

TobiasZawada
Copy link
Collaborator

Mark kw-replacment overlays as such and remove only marked overlays in adoc-unfontify-region-function.

@TobiasZawada
Copy link
Collaborator Author

The integration test error is already fixed in !45.

;; with many (minor) modes using overlays such as flyspell or ediff
(when adoc-insert-replacement
(remove-overlays beg end))
(cl-loop for ol being the overlays from beg to end
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a big fan of cl-loop (as it tends to confuse people not familiar with CL), so I'd suggest replacing it with a different looping construct.

@TobiasZawada TobiasZawada merged commit d4df8e1 into master Feb 1, 2024
8 checks passed
@bbatsov
Copy link
Owner

bbatsov commented Feb 1, 2024

Small remark for the future - please squash related commits, as now you polluted the git history with a bunch of meaningless commits (e.g. the last 4 commits). Such commits make the history harder to interact with, so they have to be avoided.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Feb 2, 2024

@bbatsov Normally, I would have splash-committed. But that would have removed our journey through the cl-loop-less variant of the interval iteration and the struggle for getting rid of the warnings. Sometimes such information can become rather handy when one encounters similar problems. In such a cases a look at the commit history helps.

That is also the reason why I did prefer a revert of my changes over a reset.

Normally, when commits really were only meaningless one-ways into dead ends I prefer to hard reset and force push in the development branch.

But, if you really dislike the current state of the history we can also reset the master, commit and force-push. Just give me a notice if you want me to do it. If you do not commit anything else in the meanwhile this does not cause any trouble. The final state of the worktree will be the same and the hick-up in the history will not be seen anymore.

To be more specific: I would reset to 7a28db1 and then commit the final state of d4df8e1.

@bbatsov
Copy link
Owner

bbatsov commented Feb 2, 2024

Sometimes such information can become rather handy when one encounters similar problems. In such a cases a look at the commit history helps.

I don't think in our particular case this information has any value as some changes were done only to be reverted in the end. For me that's just noise, especially given that those were not even released.

It's a very bad idea to force push changes to master (you don't know who might have pulled the code by now), so let's leave the code as is. Just be more careful with the merges going forward.

@TobiasZawada
Copy link
Collaborator Author

TobiasZawada commented Feb 2, 2024

BTW:

It's a very bad idea to force push changes to master (you don't know who might have pulled the code by now), so let's leave the code as is. Just be more careful with the merges going forward.

This would only matter if there really was somebody who cloned or pulled in the time gap from the merge of the "faulty commit" to the merge of the "splash commit".
But yes, damn you are right. One shouldn't do that in collaborative work.

@bbatsov
Copy link
Owner

bbatsov commented Feb 2, 2024

Keep in mind that MELPA and ELPA-devel pull from the repo automatically every few hours. (although I'm not sure if they don't do clean clones if they encounter errors or whatever)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants