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

Update protocols.adoc #654

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update protocols.adoc #654

wants to merge 2 commits into from

Conversation

ieugen
Copy link

@ieugen ieugen commented Jul 15, 2023

Made sure this is mentioned as first argument.
It's not obvious from the docs.

  • Have you read the guidelines for contributing?
  • Have you signed the Clojure Contributor Agreement?
  • Have you verified your asciidoc markup is correct?

Signed CA on 2022-11-16 .

Made sure `this` is mentioned as first argument.
It's not obvious from the docs.
@seancorfield
Copy link
Member

Because you've added an extra argument, the example calls are no longer correct.

Perhaps renaming the current first argument to this would be a safer change? (although that would make the argument lists be this b and this b c which is less intuitive in my opinion)

@ieugen
Copy link
Author

ieugen commented Jul 16, 2023

I can prepend this and drop the last argument.

@ieugen
Copy link
Author

ieugen commented Jul 16, 2023

@seancorfield : If it's still not good, please feel free to update it so it is fine.

@seancorfield
Copy link
Member

The protocol P / bar-me code is still incorrect because you've added an argument there.

I'll leave it up to @puredanger et al to decide what clarifications actually work / are needed here.

@puredanger
Copy link
Member

There are a couple of existing issues, #215 and #216, that I think also have some excellent points and examples. I think the most important thing to change about the examples here is to make the examples actually meaningful, and not use foo/bar/baz at all. Or maybe it's syntax + examples or something.

My one hesitation with making this more prevalent (even though this is a common usage), is that it implies extra meaning for developers coming from Java (where that is a special thing), and also the anaphoric macro proxy where this actually is a "special" bound symbol. But maybe it's just a matter of saying that explicitly in the doc.

@ieugen
Copy link
Author

ieugen commented Jul 18, 2023

I think keeping this (for a lack of better name) + explicit mentioning behavior in the docs is good enough.
I could try to merge the docs.
@puredanger: please let me know if you want to take this on or not.

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.

3 participants