Clarify ConstNodeInspector code #39

doodzik · 2020-10-21T20:26:03Z

What are you trying to accomplish?

I was going through the code to implement #15 and I had a hard time following the code in the ConstNodeInspector class. As such I refactored it to be a bit more self-explanatory.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code refactor (non-breaking change that doesn't add functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist

It is safe to simply rollback this change.

doodzik · 2020-10-23T01:33:20Z

lib/packwerk/association_inspector.rb

 require "packwerk/node"

 module Packwerk
  # Extracts the implicit constant reference from an active record association
  class AssociationInspector
-    include ConstantNameInspector


I found it really confusing that the interfaces aren't postfixed with interface and made it hard to understand the code initially.

doodzik · 2020-10-23T01:35:18Z

lib/packwerk/const_node_inspector.rb

+      namespace_path = Node.enclosing_namespace_path(node, ancestors: ancestors)
+      constant_name = Node.constant_name(node)
+      namespace_path.push(constant_name).join("::")
+    end


Some of these private methods could live in another class, but tackling that issue in this PR would increase the scope of the PR too much and make it harder to review.

doodzik · 2020-10-23T01:37:54Z

lib/packwerk/reference_extractor.rb

@@ -17,7 +17,7 @@ class ReferenceExtractor
    sig do
      params(
        context_provider: Packwerk::ConstantDiscovery,
-        constant_name_inspectors: T::Array[Packwerk::ConstantNameInspector],


When I did my first pass through the code, not having the interface postfix for this constant made the code harder to understand. So I'm adding one here.

tomstuart · 2020-10-23T08:20:32Z

Ditto my comment on #42 about commits.

Also, do we normally use an …Interface suffix for modules which provide Sorbet interfaces?

exterm · 2020-10-23T17:42:06Z

do we normally use an …Interface suffix for modules which provide Sorbet interfaces?

I don't think a convention exists, at least at Shopify, and I find it very confusing. IMO, we should have a different method to include it (implement instead of include), but that would have to be supplied by sorbet.

The problem is not in the interface definition; it's obvious from the first few lines of the modules that it's an interface. It's in implementing the interface. So having a more specific method to do that would be better than an Interface suffix on the class name IMO. We don't use Class and Module suffixes either.

doodzik · 2020-10-23T18:33:38Z

I don't think a convention exists, at least at Shopify, and I find it very confusing. IMO, we should have a different method to include it (implement instead of include), but that would have to be supplied by sorbet.

The problem is not in the interface definition; it's obvious from the first few lines of the modules that it's an interface. It's in implementing the interface. Having a more specific method to do that would be better than an Interface suffix on the class name IMO.

@exterm We don't have a company convention around this, but in my previous team, it was a team convention. My guiding principle here is that we want to write our code that is easy to understand. When reading the code that includes an interface, it is not clear what is happening. We could either include methods from a module or implement an interface. Granted that this is a minor inconvenience, those add up and contribute to code that is hard to follow.

Having a dedicated keyword would be the best solution, but that doesn't strike me as the most practical solution here. I could try to add it to sorbet if this is something we desire. @Morriar Do you think this would be a good addition?

We don't use Class and Module suffixes either.

Yes, we don't suffix Classes nor Modules, but those constructs are first-class citizens in Ruby. Interfaces are not. We are following similar conventions for constructs like Controllers, Services, and Views. The only exceptions are Models, which is possible because every other class is suffixed in rails and the models live in a dedicated model folder.

doodzik · 2020-10-27T16:53:04Z

I opened an issue for the interface issue on sorbet sorbet/sorbet#3578. For the time being, I'm going to drop the changes to the interface in the PR. I also split the commit into multiple commits as @tomstuart suggested.

tomstuart

Thank you for doing this @doodzik! It’s a positive change and I’m glad to see the code becoming clearer.

In a few places I’ve added some historical context which I hope helps you to decide whether you want to commit to these changes. It’s all just food for thought; let me know what you think and I’ll come back and approve if you’re happy.

tomstuart · 2020-10-28T09:55:05Z

lib/packwerk.rb

@@ -14,7 +14,6 @@
 require "packwerk/configuration"
 require "packwerk/const_node_inspector"
 require "packwerk/constant_discovery"
-require "packwerk/constant_name_inspector"


I can’t decide whether this require is unnecessary because I don’t really know what any of the requires in lib/packwerk.rb are for. Is the intention for it to require everything in the “public API” of Packwerk so that anyone using the gem can refer to e.g. Packwerk::Inflections without needing to require it themselves? If so, maybe lots of these requires are unnecessary because they’re loading Packwerk internals; but conversely, if the goal is to have a single file which loads every class & module in Packwerk to ensure everything is available, it seems simplest to exhaustively list them all here rather than try to keep track of which ones recursively require which other ones (since that can change over time).

@exterm, do you have any context on the intention here?

I prefer requiring a file where it is actually used because it makes the dependencies clearer. In this case, the requires in lib/package.rb would only contain the entry points into the application and what we intend to expose to our users. This also means that you will not get into a situation where you don't know what any of the requires is for.

I think there is an argument for convenience by requiring every file in the lib/package.rb file, but I think having clearly defined requires makes the codebase easier to understand in the long run.

I would like us to align one approach since this allows us to be consistent in the codebase, but I'm happy to go with either approach. So thank you for raising this issue :)

tomstuart · 2020-10-28T10:10:03Z

lib/packwerk/const_node_inspector.rb

@@ -9,12 +9,12 @@ class ConstNodeInspector
    include ConstantNameInspector

    def constant_name_from_node(node, ancestors:)
-      return nil unless Node.type(node) == Node::CONSTANT
+      return nil unless Node.constant?(node)


There’s a fair amount of discussion about this design decision on the (Shopify-only) Shopify/packwerk-old#266 PR. The idea is that there were many places in Packwerk where we wanted to test for multiple node types, e.g. “if this is a class or module or constant assignment, then …, otherwise …”, so the API was designed to be usable in a case statement:

case Node.type(node) when Node::CLASS, Node::MODULE, Node::CONSTANT_ASSIGNMENT # … when Node::BLOCK # … else # … end

If we provide predicates instead then this becomes a slightly less convenient if Node.class?(node) || Node.module?(node) || Node.constant_assignment?(node). I was reluctant to provide both because that would mean our Packwerk::Node API offers two different ways of doing the same thing, which makes it (again slightly) less straightforward to use.

Looking at the code now, I can see that essentially all of these case statements have been moved into Packwerk::Node itself, so perhaps there’s a case for making Node.type private and exposing only predicates to the rest of Packwerk. WDYT?

API offers two different ways of doing the same thing

Agreed, I think it makes sense to keep the API as straightforward to use as possible.

Looking at the code now, I can see that essentially all of these case statements have been moved into Packwerk::Node itself, so perhaps there’s a case for making Node.type private and exposing only predicates to the rest of Packwerk.

I did a quick experiment to verify that we use it only in Packwerk::Node, and we do. So it shouldn't be an issue to change Node.type to be private.

Looking at the Node Module and how it uses the switch statements, it seems to me that we are missing a Factory here. I would suggest a separate PR to move as much logic out of the Node module into dedicated modules, converting the Node module into a Factory module. And then, as you mentioned, change the Node.type method to be private. This would clean up the code and simplify the API of Packwerk::Node WDYT?

Looking at the Node Module and how it uses the switch statements, it seems to me that we are missing a Factory here. I would suggest a separate PR to move as much logic out of the Node module into dedicated modules, converting the Node module into a Factory module.

When we discussed this on a call, I think we agreed that a) it’s a good idea but b) you’re not going to do it at the moment.

tomstuart · 2020-10-28T10:20:04Z

lib/packwerk/const_node_inspector.rb

@@ -40,5 +35,19 @@ def constant_in_module_or_class_definition?(node, parent:)
        parent_name && parent_name == Node.constant_name(node)
      end
    end
+
+    def fully_qualify_constant(node, ancestors:)


Extracting these private methods is a good idea. I would love to suggest better names for them to more clearly describe what they’re doing, but I actually don’t understand what the code does in the first place (why is it sometimes returning a “parent module name” generated only from the ancestor nodes, but sometimes a ”qualified constant” name generated from the node itself?) so can’t begin to think of anything better. Fixing the deeper problem is a job for another PR. 😄

Agreed, I'm not confident in why the code does what it does. I think it would be good to go through the test cases and document how the data gets manipulated in those methods. I would add to this that we can probably extract those private methods into an Ancestor module in a separate PR and investigate and clarify how it works there. WDYT?

Again, it’s a good idea — let’s decide separately whether it’s worth doing in another PR.

tomstuart · 2020-10-28T10:28:23Z

lib/packwerk/const_node_inspector.rb

      # Only process the root `const` node for namespaced constant references. For example, in the
      # reference `Spam::Eggs::Thing`, we only process the const node associated with `Spam`.
-      parent = ancestors.first
-      return nil if parent && Node.constant?(parent)
+      return nil if root_constant?(ancestors)


The name root_constant? is the opposite of what this check is doing — as the comment says, we want to ignore every node except Spam in Spam::Eggs::Thing, so we return nil if we find ourselves trying to process a node like Eggs or Thing whose parent is another constant. If you want to keep the name root_constant? then the conditional should be unless root_constant? and the behaviour of the predicate should be reversed.

tomstuart · 2020-10-28T10:31:56Z

lib/packwerk/const_node_inspector.rb


-      if constant_in_module_or_class_definition?(node, parent: parent)
+      if constant_in_module_or_class_definition?(node, ancestors: ancestors)


I don’t think this part of the change is worth it, because we’re now intentionally passing more information into constant_in_module_or_class_definition? than it needs to do its job. For the caller it’s useful to be able to see at a glance that this decision depends only upon the parent node, not the whole ancestor chain, and for the implementer it’s helpful to be prevented from accidentally poking around in the rest of the ancestor chain unless you really mean it (and deliberately change the arguments to reflect that). WDYT?

Agreed, I'm providing too much in the private method and making the intention behind the code harder to understand 👍

tomstuart · 2020-10-28T10:45:19Z

lib/packwerk/const_node_inspector.rb

      end
+    rescue Node::TypeError


I’m on the fence about this one. The rescue was originally added to deal with an occurrence of self.class::HEADERS in core. I agree that Node.constant_name can raise TypeError elsewhere in this method but that’s not something we’ve seen happen so I’m not able to decide whether returning nil would be the right thing to do in those unknown cases.

I'm going to remove this change from the PR because another PR was opened that deals with this issue more concretely ref #54

I'm removing the require statement for ConstantNameInspector in the main packwerk file. The class is require at the file that uses it and doesn't need to be specified a second time.

I'm adding a `.constant?` class method to the Node class to check if a node instance is a constant or not. Before this change we would use `Node.type(node) == Node::CONSTANT` to check if a node is a constant. This however exposes too many internals and can be abstracted into a class method, which makes the code easier to understand.

I'm moving a chunk of code out of the constant_name_from_node method. This makes the intend clearer.

tomstuart

Looks good. Thanks for your patience! 🙏

tomstuart · 2020-11-04T10:12:48Z

lib/packwerk/const_node_inspector.rb

+    # Only process the root `const` node for namespaced constant references. For example, in the
+    # reference `Spam::Eggs::Thing`, we only process the const node associated with `Spam`.
+    def root_constant?(parent)
+      !parent || !Node.constant?(parent)


For me this would be 1% easier to understand as !(parent && Node.constant?(parent)) (per de Morgan) but there’s not much in it.

Morriar · 2020-11-04T20:23:16Z

Having a dedicated keyword would be the best solution, but that doesn't strike me as the most practical solution here. I could try to add it to sorbet if this is something we desire. @Morriar Do you think this would be a good addition?

I agree with the answer given by the Sorbet team on this: it would be hard to enforce using the static checker for retro-compatibility purposes so we would end up with the two forms in our codebase which would be super confusing for the developers.

Add pronounciation to README

doodzik force-pushed the clarify_const_node_inspector branch 2 times, most recently from cd73672 to d24769d Compare October 23, 2020 00:20

doodzik changed the title ~~clarify code of ConstNodeInspector~~ Clarify ConstNodeInspector code Oct 23, 2020

doodzik self-assigned this Oct 23, 2020

doodzik commented Oct 23, 2020

View reviewed changes

doodzik marked this pull request as ready for review October 23, 2020 01:33

doodzik requested a review from a team as a code owner October 23, 2020 01:33

doodzik commented Oct 23, 2020

View reviewed changes

tomstuart mentioned this pull request Oct 23, 2020

Add Checker Interface for PrivacyChecker and DependencyChecker #43

Merged

6 tasks

doodzik force-pushed the clarify_const_node_inspector branch 2 times, most recently from 8410e30 to 30ed5b1 Compare October 27, 2020 16:50

doodzik requested a review from tomstuart October 27, 2020 16:51

doodzik force-pushed the clarify_const_node_inspector branch from 30ed5b1 to 23d4c49 Compare October 27, 2020 16:54

tomstuart reviewed Oct 28, 2020

View reviewed changes

tomstuart mentioned this pull request Oct 28, 2020

Clarify AssociationInspector class #42

Merged

6 tasks

doodzik added 3 commits October 29, 2020 17:30

Remove uneccassary require statement.

8aee767

I'm removing the require statement for ConstantNameInspector in the main packwerk file. The class is require at the file that uses it and doesn't need to be specified a second time.

add fully_qualify_constant private method

f9941e7

I'm moving a chunk of code out of the constant_name_from_node method. This makes the intend clearer.

doodzik force-pushed the clarify_const_node_inspector branch from 23d4c49 to 71cee51 Compare October 30, 2020 00:47

doodzik requested a review from tomstuart November 3, 2020 16:11

tomstuart approved these changes Nov 4, 2020

View reviewed changes

cleanup guard method to have conditional in private method

ea2e68a

doodzik force-pushed the clarify_const_node_inspector branch from 71cee51 to ea2e68a Compare November 5, 2020 00:21

doodzik merged commit dac7b10 into main Nov 5, 2020

doodzik deleted the clarify_const_node_inspector branch November 5, 2020 00:26

doodzik mentioned this pull request Nov 5, 2020

Simplify public interface of the Node class #62

Merged

6 tasks

wildmaples temporarily deployed to rubygems November 16, 2020 21:40 Inactive

shioyama pushed a commit that referenced this pull request May 21, 2021

Merge pull request #39 from Shopify/pronounciation

3e296a0

Add pronounciation to README

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify ConstNodeInspector code #39

Clarify ConstNodeInspector code #39

doodzik commented Oct 21, 2020 •

edited

Loading

doodzik Oct 23, 2020

doodzik Oct 23, 2020 •

edited

Loading

doodzik Oct 23, 2020

tomstuart commented Oct 23, 2020 •

edited

Loading

exterm commented Oct 23, 2020 •

edited

Loading

doodzik commented Oct 23, 2020 •

edited

Loading

doodzik commented Oct 27, 2020

tomstuart left a comment

tomstuart Oct 28, 2020

doodzik Oct 30, 2020

tomstuart Oct 28, 2020

doodzik Oct 30, 2020 •

edited

Loading

tomstuart Nov 4, 2020

tomstuart Oct 28, 2020

doodzik Oct 30, 2020 •

edited

Loading

tomstuart Nov 4, 2020

tomstuart Oct 28, 2020

tomstuart Oct 28, 2020

doodzik Oct 30, 2020

tomstuart Oct 28, 2020

doodzik Oct 30, 2020 •

edited

Loading

tomstuart left a comment

tomstuart Nov 4, 2020

Morriar commented Nov 4, 2020


		if constant_in_module_or_class_definition?(node, parent: parent)
		if constant_in_module_or_class_definition?(node, ancestors: ancestors)

Clarify ConstNodeInspector code #39

Clarify ConstNodeInspector code #39

Conversation

doodzik commented Oct 21, 2020 • edited Loading

What are you trying to accomplish?

Type of Change

Checklist

Choose a reason for hiding this comment

doodzik Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomstuart commented Oct 23, 2020 • edited Loading

exterm commented Oct 23, 2020 • edited Loading

doodzik commented Oct 23, 2020 • edited Loading

doodzik commented Oct 27, 2020

tomstuart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doodzik Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doodzik Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doodzik Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

tomstuart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morriar commented Nov 4, 2020

doodzik commented Oct 21, 2020 •

edited

Loading

doodzik Oct 23, 2020 •

edited

Loading

tomstuart commented Oct 23, 2020 •

edited

Loading

exterm commented Oct 23, 2020 •

edited

Loading

doodzik commented Oct 23, 2020 •

edited

Loading

doodzik Oct 30, 2020 •

edited

Loading

doodzik Oct 30, 2020 •

edited

Loading

doodzik Oct 30, 2020 •

edited

Loading