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

Remove the deprecated clear modifier. #18887

Merged
merged 2 commits into from Apr 5, 2024

Conversation

ppedrot
Copy link
Member

@ppedrot ppedrot commented Apr 3, 2024

It has been deprecated since 8.17, long enough for people to have realized this.

@ppedrot ppedrot added the kind: cleanup Code removal, deprecation, refactorings, etc. label Apr 3, 2024
@ppedrot ppedrot added this to the 8.20+rc1 milestone Apr 3, 2024
@ppedrot ppedrot requested a review from a team as a code owner April 3, 2024 11:15
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Apr 3, 2024
@ppedrot ppedrot added the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 3, 2024
@ppedrot ppedrot force-pushed the rm-deprecated-clear-modifier branch from 121a671 to ff58952 Compare April 3, 2024 11:17
@coqbot-app coqbot-app bot removed request: full CI Use this label when you want your next push to trigger a full CI. needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. labels Apr 3, 2024
@ppedrot
Copy link
Member Author

ppedrot commented Apr 3, 2024

I will write a cleanup following this removal, but I prefer having this PR separate.

@SkySkimmer SkySkimmer self-assigned this Apr 3, 2024
@herbelin
Copy link
Member

herbelin commented Apr 3, 2024

I may have missed a discussion, but what are the objections against a clear modifier? It seems to me to be a useful feature.

It has been deprecated since 8.17, long enough for people to have realized
this.
@ppedrot
Copy link
Member Author

ppedrot commented Apr 4, 2024

@herbelin see #16407

@ppedrot ppedrot added the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 4, 2024
@ppedrot ppedrot force-pushed the rm-deprecated-clear-modifier branch from ff58952 to c4e2ac0 Compare April 4, 2024 13:51
@ppedrot ppedrot requested a review from a team as a code owner April 4, 2024 13:51
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Apr 4, 2024
@herbelin
Copy link
Member

herbelin commented Apr 4, 2024

The discussion was more about the syntactic and semantics difference between ssr and vanilla for the clear feature, not about the principle of such feature. I feel uncomfortable to take such kind of decision of removal of an a-priori useful feature without a prior vision of where we are going in general regarding tactics. Also, I'd like to remind that if I did not document and advertise the clear flag further, and did not fully develop it, it was precisely in the hope of a discussion about what is our vision about tactics (especially after the trauma that the implementation of the % intropattern created).

My perception is that we are postponing for many years the question of what is our model regarding tactics. I'm unsure we can afford continuing postponing it. I hope the discussion on the long-term roadmap will help us communicating between us.

@@ -0,0 +1,4 @@
- **Removed:**
the clear modifier which was deprecated since 8.17
Copy link
Member

@jfehrle jfehrle Apr 4, 2024

Choose a reason for hiding this comment

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

Suggested change
the clear modifier which was deprecated since 8.17
the :n:`clear` modifier which has been deprecated since 8.17

Copy link
Member Author

Choose a reason for hiding this comment

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

Despite having a similar behaviour, this is not the clear tactic, so I'm not sure we want to do this. The previous changelog entry did not have a special rendering for this word (see #16407).

@herbelin
Copy link
Member

herbelin commented Apr 5, 2024

PS: My previous remarks applied to the code of the clear modifier. Deactivating the parsing rule is consistent with #16407.

@ppedrot
Copy link
Member Author

ppedrot commented Apr 5, 2024

@herbelin so there is no problem with this PR per se then?

@SkySkimmer
Copy link
Contributor

@coqbot merge now

@coqbot-app coqbot-app bot merged commit ac14145 into coq:master Apr 5, 2024
5 of 6 checks passed
@ppedrot ppedrot deleted the rm-deprecated-clear-modifier branch April 5, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup Code removal, deprecation, refactorings, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants