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

Document CursorEffectContainer and update outdated name #6441

Merged

Conversation

voidedWarranties
Copy link
Contributor

No description provided.

@frenzibyte
Copy link
Member

frenzibyte commented Dec 1, 2024

I'm not sure I see the point in this PR. If there are no follow-up changes containing actual useful changes following up on the presence of this documentation then there's no point in PR'ing this.

@frenzibyte frenzibyte closed this Dec 1, 2024
@voidedWarranties
Copy link
Contributor Author

Sorry, but I don't understand why this was closed outright.

If the question is why I decided to document this class, then it's because I am working on generic text selection (possibly towards an implementation of #1618) and ended up deriving this class and encountering the non-trivial behavior I documented in this PR.

That being said, I don't see how that is relevant to this PR's merit. The beginning and end of this changeset is that I am trying to improve the code quality of CursorEffectContainer and allow future developers/consumers of framework to easily use/understand it by adding missing documentation. In my opinion, for changes like this, there shouldn't be any need for further justification.

Sorry if I am off base, I just want to understand the reasoning here.

@frenzibyte
Copy link
Member

frenzibyte commented Dec 2, 2024

If there are no follow-up changes or promises of follow-up changes in the OP, then it becomes redundant to push such changes and expect it to be reviewed or merged, especially with no description provided. It looks like a random PR not adding anything helpful.

State your reasoning, even if it's "allow future developers/consumers of framework to easily use/understand", that doesn't make the PR look like it's not doing anything.

Reopening as there is some real intent to this change.

@peppy peppy merged commit 0665828 into ppy:master Dec 2, 2024
25 checks passed
@voidedWarranties voidedWarranties deleted the cursoreffectcontainer-code-quality branch December 2, 2024 05:26
@bdach
Copy link
Collaborator

bdach commented Dec 2, 2024

If there are no follow-up changes containing actual useful changes following up on the presence of this documentation then there's no point in PR'ing this

Huh? Where is this coming from? When has any of us ever said this? Baffled by this reaction as well.

Documentation is always welcome if someone is willing to contribute it.

@frenzibyte
Copy link
Member

I hope you understand where I'm coming from here, all I'm seeing is a PR out of nowhere just adding comments over a class and moving on. If you don't then it's fine, I will leave such PRs alone, sure.

@peppy
Copy link
Member

peppy commented Dec 3, 2024

A quick check that the PR is of good quality and merge takes much less time than what happened in this thread as a result of closing it. And resulted in better code quality, so seems like a win-win to not go against the grain and raise questions.

Putting yourself in the PR opener's shoes can sometimes help!

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

Successfully merging this pull request may close these issues.

4 participants