Mark non-documented API as explicitly private #276

alexevanczuk · 2022-12-06T13:44:28Z

What are you trying to accomplish?

This PR addresses a portion of #275.

In this PR, I mark many constants as explicitly private API.

PR is best reviewed commit by commit with hide whitespace changes set to true.

Alternate implementation: https://github.com/Shopify/packwerk/pull/282/commits

What approach did you choose and why?

I used private_constant as the vanilla Ruby way to mark something as private. I did this instead of something like :nodoc:, which requires folks to either read the source code or look at code with YARD. private_constant gives the user unavoidable feedback when their code is executed.

I separated out the main autoloads into two lists – one to autoload public API and one to autoload private API. This is mostly for the benefit of maintainers to easily see what APIs need to stay supported and what can change.

As a result of this, I needed to change references to these constants to omit the leading Packwerk namespace, which would constitute a private (and therefore not allowed) reference.

I had an alternate idea, which was to create a module Packwerk::Private where we could move all of our constants. With this approach, we could have a single private_constant :Private call that would mark everything as private. I like this pattern because it would mean all public API would be in one place that can easily be seen, and private things would be in their own explicit location. I'd be happy to apply this pattern instead if other maintainers prefer it.

Here is that implementation: https://github.com/Shopify/packwerk/pull/282/commits

What should reviewers focus on?

Would folks prefer to move all private implementation into one place, as described above?
Is there any private API that should be public?
Is there any public API that should be private?

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

Breaking change (fix or feature that would cause existing functionality to change)

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

This is a breaking change because we've marked a lot of things as private. I've left many things as public where we know we are exposing a public API, but its possible some folks are using some of these constants that are currently private. My hope was that they could release an issue against packwerk 3.0 if they have a use-case for these private constants.

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

alexevanczuk · 2022-12-06T13:52:45Z

lib/packwerk.rb

 module Packwerk
  extend ActiveSupport::Autoload

+  # Public APIs


I chose these APIs with these rationales:

Checker

API for extending checkers

Cli

Main API for calling packwerk right now. Eventually we might want API like Packwerk.check instead of using the CLI, but this is the de-facto API.

Offense

Part of the interface for OffensesFormatter

OffensesFormatter

API for extending offenses formatter

OutputStyle

Implementation of this needs to be passed into CLI

Package, PackageSet, PackageTodo

API for getting information about YML files

Parsers

Contains documented API for Packwerk::Parsers::Factory.erb_parser_class=(...), which is the current API for substituting the ERB parser.
The hope is that #243 will allow us to mark this API as private.

This module also contains ParserInterface as well. With the above PR, we'd allow the interface to be public but the rest to be private.

Reference, ReferenceOffense

Part of the API to Checker

Result

Return value of CLI

Result is overloaded, I vote to nest the toplevel result in CLI. We should also nest the app validator result in Validator instead, especially if we use it in the interface.

Yeah makes sense, changed!

alexevanczuk · 2022-12-06T13:53:07Z

lib/packwerk.rb

+
+  # Private APIs
+  # Please submit an issue if you have a use-case for these
  autoload :ApplicationLoadPaths


This is more easily read commit by commit. A previous commit sorts these then another separates them.

gmcgibbon

I agree with the shape of this API, but I think we need to label each class as @api private and make sure the public ones are documented (at least at the class/module level). Though, maybe private_constant does this for us. @rafaelfranca do you have an opinion on using private_constant here?

Also, can we confirm private_constant works on autoloaded constants without loading them?

alexevanczuk · 2022-12-12T14:29:27Z

I agree with the shape of this API, but I think we need to label each class as @api private and make sure the public ones are documented (at least at the class/module level). Though, maybe private_constant does this for us. @rafaelfranca do you have an opinion on using private_constant here?

private_constant does this for us insofar as if a user attempts to access the constant (e.g. Packwerk::MyPrivateConstant they will get a ruby runtime error (and a sorbet static error) once that reference attempts to resolve. I'm not sure if @api private serves any other useful purpose for us (perhaps its a directive that YARD reads somehow, although I'd hope that YARD or other systems also read private_constant).

Happy to use another construct if the team prefers and would love to learn about the pros/cons as you see them.

I'm also generally really curious what you think of the Packwerk::Private module. This would let us make a single private_constant :Private call and then everything under it would be default private. That would be my preference here and would also organize the API for the consumer into a bucket of private stuff and a bucket of non-private stuff, which I think will help readability and API maintenance. Let me know what you think.

Also, can we confirm private_constant works on autoloaded constants without loading them?

Yep -- I verified that. You can see my verification in the commit history, where I throw a raise in a dummy class that is autoloaded and then made private. I'll delete those commits before merging.

alexevanczuk · 2022-12-12T14:57:01Z

@gmcgibbon

I created the alternate implementation where I nest everything under Private here: #282

Let me know what you think! The diff is a bit messy when not viewed commit by commit since it looks like a bunch of files are deleted/renamed, so its easier to just review the last four commits (since that PR is based off of this one) one by one.

gmcgibbon · 2022-12-13T19:58:31Z

USAGE.md

 While `packwerk` ships with its own offense formatter, you may specify a custom one in your configuration file via the `offenses_formatter:` key.  Your custom formatter will be used when `bin/packwerk check` is run.

-Firstly, you'll need to create an `OffensesFormatter` class that includes `Packwerk::OffensesFormatter`. You can use [`Packwerk::Formatters::OffensesFormatter`](lib/packwerk/formatters/offenses_formatter.rb) as a point of reference for this. Then, in the `require` directive described above, you'll want to tell `packwerk` about it:
+Firstly, you'll need to create an `OffensesFormatter` class that includes `Packwerk::OffensesFormatter`. You can use [`Formatters::OffensesFormatter`](lib/packwerk/formatters/offenses_formatter.rb) as a point of reference for this. Then, in the `require` directive described above, you'll want to tell `packwerk` about it:


I think this should be reverted. We should fully qualify constants in the USAGE doc.

For sure, I think this just came from an errant find and replace.

gmcgibbon · 2022-12-13T20:50:15Z

lib/packwerk.rb

+  # Private APIs
+  # Please submit an issue if you have a use-case for these
  autoload :ApplicationLoadPaths
+  private_constant :ApplicationLoadPaths


I think we should move these declarations to the files they are loading. This is because if we want to refactor to use a documentation tool to define our API boundaries we can more easily do so (instead of referencing the main entrypoint, it is clear via search that a module/file is private).

So what I'm saying is I think this line should be moved to lib/packwerk/application_load_paths.rb under the class definition.

gmcgibbon · 2022-12-13T20:58:54Z

test/parser_test_helper.rb

-  class << self
-    def parse(source)
-      Packwerk::Parsers::Ruby.new.call(io: StringIO.new(source))
+module Packwerk


This needs to be in a packwerk folder. Same with all the other test helpers that have packwerk module nesting. This one should also be moved to support.

Mind if we move test files in a subsequent PRs?

Yes, this is noisy enough, we can do it later.

gmcgibbon · 2022-12-13T21:04:17Z

test/unit/application_validator_test.rb

    private

-    sig { returns(Packwerk::ApplicationValidator) }
+    sig { returns(ApplicationValidator) }


It looks like unit tests also are missing the toplevel packwerk folder to match the module nesting. We aren't autoloading our tests, but this would make it easier if we ever chose to.

Same comment – let me know if you're okay with us moving test files in another PR.

gmcgibbon · 2022-12-13T21:05:50Z

lib/packwerk.rb

  autoload :ParseRun
-  autoload :UnresolvedReference
-  autoload :Reference
+  private_constant :ParseRun


I've always wondered why this is called ParseRun. Should we just rename it to Run? It is private API so we can do this for free later if need be.

Not sure about that one either. Personally, my preference would be to just have API on packwerk directly, e.g.

Packwerk.check Packwerk.update_todo

This feels like the best UX to me.

It would be, but these methods aren't called in code, so I don't think toplevel access matters much. We can also potentially do this later.

gmcgibbon · 2022-12-13T21:09:39Z

I created the alternate implementation where I nest everything under Private here: #282

I think it makes sense to keep the approach in this PR (#276). In my opinion, naming and visibility should be separate concerns. While I understand and appreciate the explicit nature of using a Private module, I think it would be best to draw these lines manually with doc tools or private_contant.

From #283, conflicts with changes in #276.

alexevanczuk requested a review from a team as a code owner December 6, 2022 13:44

alexevanczuk commented Dec 6, 2022

View reviewed changes

alexevanczuk force-pushed the ae-mark-things-as-private branch from 88a66c6 to 3440c34 Compare December 9, 2022 18:49

gmcgibbon reviewed Dec 9, 2022

View reviewed changes

alexevanczuk force-pushed the ae-mark-things-as-private branch from 3440c34 to 0b584f6 Compare December 12, 2022 14:16

alexevanczuk mentioned this pull request Dec 12, 2022

Create top-level Private module for nesting private constants #282

Closed

gmcgibbon reviewed Dec 13, 2022

View reviewed changes

gmcgibbon approved these changes Dec 14, 2022

View reviewed changes

alexevanczuk force-pushed the ae-mark-things-as-private branch from beacfea to c5bc226 Compare December 14, 2022 16:34

Mark private API as constant

e1da318

alexevanczuk force-pushed the ae-mark-things-as-private branch from eaeac74 to e1da318 Compare December 14, 2022 17:29

alexevanczuk merged commit 45e80f9 into main Dec 14, 2022

alexevanczuk deleted the ae-mark-things-as-private branch December 14, 2022 17:36

gmcgibbon added a commit that referenced this pull request Dec 14, 2022

Fix stale Validator::Result reference

af7b0aa

From #283, conflicts with changes in #276.

alexevanczuk mentioned this pull request Dec 15, 2022

Move test files so that file structure matches constant nesting structure #287

Merged

6 tasks

davidstosik mentioned this pull request Feb 9, 2023

Remove empty_commit file, as it seems unnecessary #302

Merged

7 tasks

gmcgibbon mentioned this pull request Mar 1, 2023

[Packwerk 3.0] Ensure API boundaries are up-to-date #275

Closed

Mark non-documented API as explicitly private #276

Mark non-documented API as explicitly private #276

Uh oh!

Conversation

alexevanczuk commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Additional Release Notes

Checklist

Uh oh!

Choose a reason for hiding this comment

Checker

Cli

Offense

OffensesFormatter

OutputStyle

Package, PackageSet, PackageTodo

Parsers

Reference, ReferenceOffense

Result

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmcgibbon left a comment

Choose a reason for hiding this comment

Uh oh!

alexevanczuk commented Dec 12, 2022

Uh oh!

alexevanczuk commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmcgibbon commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexevanczuk commented Dec 6, 2022 •

edited

Loading