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

[cmd] Fix proxy command deprecation docs #7396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spacey-sooty
Copy link
Contributor

No description provided.

@spacey-sooty spacey-sooty requested a review from a team as a code owner November 15, 2024 02:43
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@rzblue
Copy link
Member

rzblue commented Nov 15, 2024

C++ has another constructor that needs changed

@spacey-sooty
Copy link
Contributor Author

/format

@spacey-sooty
Copy link
Contributor Author

CI failure seems to be unrelated.

@spacey-sooty
Copy link
Contributor Author

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

Not applicable to python, the deprecation message is different.

@rzblue
Copy link
Member

rzblue commented Nov 20, 2024

Just remembered, the other thing that needs fixed is the docs for Commands.deferredProxy

@spacey-sooty
Copy link
Contributor Author

I'd be inclined to undeprecate deferred proxy and leave it with the new implementation. I feel it accurately describes what its syntactic sugar over

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Why not use "proxy a deferred command" in the C++ docs like we do in Java? (Also, while we're here, it might be nice to link to command factories in adddition to the command class)

@spacey-sooty
Copy link
Contributor Author

Why not use "proxy a deferred command" in the C++ docs like we do in Java?

You'll notice this updates the Java documentation as well, that documentation was wrong and did not give the correct behaviour. This change corrects it to give accurate behaviour

(Also, while we're here, it might be nice to link to command factories in adddition to the command class)

Yes I plan to do this but I'll also have to undeprecate deferredProxy first :)

@KangarooKoala
Copy link
Contributor

Why not use "proxy a deferred command" in the C++ docs like we do in Java?

You'll notice this updates the Java documentation as well, that documentation was wrong and did not give the correct behaviour. This change corrects it to give accurate behaviour

Sorry I got the order wrong when writing on mobile, but the changed Java docs say "defer a proxy command", not "use defer command on a proxy command". Regardless of which one we use, it should be consistent between Java and C++. (And my vote is for the Java version since it's less tied to the particular command types)

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.

4 participants