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

Protocol fun documentation parity #800

Merged
merged 4 commits into from
Sep 21, 2022
Merged

Conversation

bobisageek
Copy link
Contributor

@bobisageek bobisageek commented Sep 19, 2022

Please answer the following questions and leave the below in as part of your PR.

Aimed at directly addressing babashka/babashka#1340

  • This PR contains a test to prevent against future regressions

Summary
Add an attribute map to the multimethods created for protocol functions. The attribute map is based directly on the signature map currently parsed by defprotocol. Also, add a test for doc output based on running the same protocol definition in clojure/jvm.

likely(?) discussion points

  • arglists aren't enforced (as in arity checks) - this is also true for 'hand-crafted' arglists, but is a variance from protocols on cl/jvm, so I wanted to call that out in case these should be captured/rendered differently
  • the "no arglist" test - I wanted to capture a variety of arglist/docstring combos for this test, and it occurred to me that it's technically possible (with SCI) to specify a protocol method with no arglists; the test is primarily there to ensure that these changes don't cause any undesired behavior around the metadata, but it might not be desirable to have a test scenario that's undesirable
  • cross-linking issues in changelog - I only saw one other example of referring to an issue from the bb issue list, but it wasn't a link; not sure if there's a more preferred style for linking over (or maybe just eschew the linking altogether)

@borkdude
Copy link
Collaborator

I'll take a look at this tomorrow

@borkdude borkdude merged commit 8330cd8 into babashka:master Sep 21, 2022
@borkdude
Copy link
Collaborator

@bobisageek I didn't see anything that looked weird to me and tested it with bb master. Thanks for the improvements.

@bobisageek bobisageek deleted the protocol-docs branch September 22, 2022 20:46
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