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 SignatureBuildOrder cop #186

Merged
merged 1 commit into from
Oct 15, 2023
Merged

Conversation

sambostock
Copy link
Contributor

Not having documentation means that rubocop:todo comments attached to the class end up erroneously treated as documentation, and the simplest way to avoid that is to document the doc, which we should be doing anyway.

See #183 for why this is relevant.

@sambostock sambostock requested a review from a team as a code owner October 13, 2023 15:26
Not having documentation means that `rubocop:todo` comments attached to
the class end up erroneously treated as documentation, and the simplest
way to avoid that is to document the doc, which we should be doing
anyway.
# - type_parameters
# - params
# - returns, or void
# - soft, checked, or on_failure
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When checking the docs for these (so I could know if they were mutually exclusive), I couldn't find the soft method in https://github.com/sorbet/sorbet/blob/4ef2b632ae1433ed1a544fd568f3481a4fc8f2df/rbi/sorbet/builder.rbi.

I did notice that there are a couple methods which we don't include in our list, which we should probably add:

  • final
  • implementation
  • bind

It might even be worth having a test that checks that the constant contains every method defined on T::Private::Methods::DeclBuilder... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

soft has been renamed into on_failure a while ago but can theoretically still be found in codebases using an old Sorbet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did notice that there are a couple methods which we don't include in our list, which we should probably add:

  • final
  • implementation
  • bind

Let's add them 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I've opened #189 to do that, to unblock this PR (and the subsequent ones branched off of it, and keep it about the documentation, especially since I need to figure out what implementation is and where it belongs 😅).

@sambostock sambostock merged commit 9f5c889 into main Oct 15, 2023
20 checks passed
@sambostock sambostock deleted the document-sig-build-order branch October 15, 2023 22:20
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.

None yet

3 participants