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

Allow for sigil declaration for publicizing of constant #35

Merged
merged 3 commits into from Oct 24, 2023

Conversation

bschrag620
Copy link
Contributor

  • Allows for declaring of a constant to be public through a comment in the file of packwerk#publicize!
  • The lookup is cached at the class level inside of Packwerk::Privacy::Checker to avoid duplicative file scans.
  • Adds logic to validator to ensure there isn't conflict between files explicitly declared as private in package.yml and explicitly declared as public by usage of the sigil.
  • Test coverage added.

@bschrag620 bschrag620 force-pushed the sigil-declaration branch 2 times, most recently from 989c457 to f993b14 Compare September 18, 2023 15:47
README.md Outdated
```ruby
module Foo
class Update
# packwerk#publicize!
Copy link
Contributor

Choose a reason for hiding this comment

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

I have two preferences for this annotation:

  1. That it lives at the top of the file, or at least within the first 5 lines of the file
  2. That it looks a bit more like sorbet (which has typed: true, for example). Maybe public_api: true?

That being said, this is one of the more meaningful, harder to change design decisions, so I wanted to get input from other folks.

cc @shageman , @technicalpickles, @ngan

I know you folks (especially Ngan) are interested in allowing things to be public via magic comments so this user is graciously helping implement it. I wanted your input on the format of that magic comment.

There are a couple of other places that will require changes related to this sigil. Namely, everything that is coupled to the public folder implementation of privacy.

In the rubyatscale org:

In gusto:

  • The published yard docs of public APIs (could be deprecated if a quick poll suggested it's not used)
  • ?? maybe other places, but its pretty greppable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alexevanczuk . The format of the annotation makes sense. I'll make that update and push it back up.

@bschrag620
Copy link
Contributor Author

@alexevanczuk Please see the latest changes and let me know your thoughts. Thanks!

@ngan
Copy link

ngan commented Sep 30, 2023

@bschrag620 thanks for doing this. I'm very interested in seeing this merged. Let me talk it over with the team next week and get back to you. We'll need to audit our things (some of which are mentioned by Alex in the inline comment).

@shageman
Copy link
Contributor

Hi @bschrag620! After some more internal discussion, I would love to approve this with one last change:

Let's replace public_api with pack_public to be a bit more specific about the context this is being used in.

I would be happy to merge your change and then update this detail or for you to update the existing PR. Let me know either way!

* fixes rubocop and sorbet checks
@bschrag620
Copy link
Contributor Author

Thanks @shageman. I've made the requested changes as well as addressed the sorbet and rubocop issues. When doing so, I did notice that the current configuration of rubocop is requiring an empty line after the magic comments, however, it doesn't recognize public_pack as a magic comment. I made note of it in the README. Currently, if you place public_pack magic comment first, rubocop doesn't seem to mind. I peaked into rubocop a bit and didn't see any sort of configurability in terms of magic comments.

@shageman shageman merged commit baa2c53 into rubyatscale:main Oct 24, 2023
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

5 participants