Add to node parsing capabilities #54

shageman · 2020-10-29T00:56:46Z

What are you trying to accomplish?

In the codebase I am working on I see several errors related to edge cases where the code parsing fails. I am working to make packwerk run.

What approach did you choose and why?

I added a bunch of defensive statements that makes packwerk continue to run even when individual nodes fail to parse.

What should reviewers focus on?

This doesn't seem like a cohesive PR at the moment. Although the tests do pass, how should I break these changes up so merging would be considered?

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.

shageman · 2020-10-29T16:54:23Z

How do I complete the CLA step?

wildmaples · 2020-10-29T19:07:39Z

How do I complete the CLA step?

@shageman https://cla.shopify.com/

lib/packwerk/parsed_constant_definitions.rb

lib/packwerk/node.rb

doodzik

Your changes look like a great addition 👍 I left two small comments but other than that it looks good.

shageman · 2020-10-29T20:07:28Z

@doodzik I believe I addressed your comments with the new changes and hope to have also appeased the linter.

shageman · 2020-10-29T20:10:16Z

Scratch that, my changes were too aggressive that broke that I need to go back and run tests locally. I hope to have another version tomorrow

doodzik · 2020-10-29T20:19:33Z

lib/packwerk/const_node_inspector.rb

      end
+    rescue Node::TypeError
+      nil


I would like to get the approval of @tomstuart because he raised an issue about defensive node parsing on one of my open PRs. From my understanding, this gem's design focuses on doing a good enough job uncovering dependency/privacy violations. In my opinion, defensive node parsing would fall under that because we don't fail if packwerk cannot handle the code. The downside is that we don't learn about parsing issues.

I totally agree. I would love to rather propose a fix for the parsing errors we are running into, but am not deep enough into packwerk and parser to understand much of what's going on.

I would be happy to pair on this or run non-defensive versions against the codebase I am looking at to give more input into what might be going on...

shageman · 2020-10-29T20:30:50Z

If I fix the linting error as proposed by the check, all tests fail: https://github.com/Shopify/packwerk/pull/54/checks?check_run_id=1328856280

exterm · 2020-10-30T15:07:15Z

all tests fail

Could you link to the build with the failing tests? the one you link to only has the failing linter.

shageman · 2020-10-30T19:33:13Z

all tests fail

Could you link to the build with the failing tests? the one you link to only has the failing linter.

Yes - no longer sure what I saw 🤔 must have been a snafu. I will update the PR

shageman · 2020-10-31T18:09:32Z

@exterm @doodzik I took another stab at the changes needed to make it work for our codebase... this time a bit more focussed and with tests.

lib/packwerk/parsed_constant_definitions.rb

lib/packwerk/node.rb

test/unit/reference_extractor_test.rb

exterm · 2020-11-09T22:13:08Z

Thank you for the adjustments! This PR is starting to look like a very valuable bugfix.

doodzik

The PR is looking good! Once Philips comment is addressed and fix the merge conflict we can ship this PR 🚢

shageman · 2020-11-12T15:50:27Z

Thanks, @doodzik - I have addressed all open comments

exterm · 2020-11-13T20:22:31Z

looking good! I'm going to test this on our core monolith.

exterm · 2020-11-13T20:58:28Z

It's complicated by the fact that packwerk main is currently broken on our monolith. We may want to fix that first before merging this. Sorry for the delay Stephan.

exterm · 2020-11-23T16:24:41Z

We fixed the main branch. @shageman , can you rebase? I'll manually test your branch on our monolith afterwards (I would have rebased myself, but there are merge conflicts and I sadly don't have the time to look into them right now)

… reference even when there are nil nodes in the namespace path

shageman · 2020-11-24T03:08:15Z

@exterm done. Thanks!

exterm · 2020-11-25T02:30:39Z

Ran it on the core monolith without problems.

exterm

Thank you for your contribution Stephan!

shageman requested a review from a team as a code owner October 29, 2020 00:56

ghost added the cla-needed label Oct 29, 2020

shageman force-pushed the main branch from 7222c47 to 42bccc5 Compare October 29, 2020 15:53

doodzik reviewed Oct 29, 2020

View reviewed changes

lib/packwerk/parsed_constant_definitions.rb Outdated Show resolved Hide resolved

doodzik reviewed Oct 29, 2020

View reviewed changes

lib/packwerk/node.rb Outdated Show resolved Hide resolved

doodzik reviewed Oct 29, 2020

View reviewed changes

ghost removed the cla-needed label Oct 29, 2020

doodzik reviewed Oct 29, 2020

View reviewed changes

shageman force-pushed the main branch from 0c79423 to b6f9859 Compare October 29, 2020 20:27

doodzik mentioned this pull request Oct 30, 2020

Clarify ConstNodeInspector code #39

Merged

6 tasks

shageman force-pushed the main branch from ffddfc1 to bb5b1f9 Compare October 31, 2020 18:02

shageman requested a review from doodzik October 31, 2020 18:02

exterm reviewed Nov 6, 2020

View reviewed changes

lib/packwerk/parsed_constant_definitions.rb Outdated Show resolved Hide resolved

shageman force-pushed the main branch from 6371b36 to 701fe0d Compare November 9, 2020 15:36

shageman changed the title ~~More defensive node parsing to prevent parsing problem crashes~~ Add to node parsing capabilities Nov 9, 2020

shageman requested a review from exterm November 9, 2020 15:45

exterm reviewed Nov 9, 2020

View reviewed changes

lib/packwerk/node.rb Outdated Show resolved Hide resolved

exterm reviewed Nov 9, 2020

View reviewed changes

test/unit/reference_extractor_test.rb Outdated Show resolved Hide resolved

doodzik approved these changes Nov 10, 2020

View reviewed changes

shageman force-pushed the main branch 2 times, most recently from 11d8945 to aa94892 Compare November 12, 2020 15:39

shageman force-pushed the main branch from f411892 to 00bf4e8 Compare November 12, 2020 15:49

shageman force-pushed the main branch from 00bf4e8 to e9d18f0 Compare November 12, 2020 15:53

shageman added 7 commits November 23, 2020 20:04

.reference_qualifications generates all possible qualifications for a…

eaa1bfd

… reference even when there are nil nodes in the namespace path

Make node.type return nil for nil nodes

dbed9c0

Prevent processing from breaking on unusual constants

a506a5a

Use each_with_object instead of reduce

f82fb17

Prevent erring out on self nodes without wide rescue

18c150a

Improve test naming for self:: node parsing fix

815637d

Remove node type protection against nil node

449b059

shageman force-pushed the main branch from e9d18f0 to 449b059 Compare November 24, 2020 03:05

exterm approved these changes Nov 25, 2020

View reviewed changes

exterm merged commit dd7c4db into Shopify:main Nov 25, 2020

exterm mentioned this pull request Mar 5, 2021

Packwerk can't parse self:: #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add to node parsing capabilities #54

Add to node parsing capabilities #54

shageman commented Oct 29, 2020

shageman commented Oct 29, 2020

wildmaples commented Oct 29, 2020

doodzik left a comment •

edited

Loading

shageman commented Oct 29, 2020

shageman commented Oct 29, 2020

doodzik Oct 29, 2020 •

edited

Loading

shageman Oct 29, 2020

shageman commented Oct 29, 2020

exterm commented Oct 30, 2020

shageman commented Oct 30, 2020 •

edited

Loading

shageman commented Oct 31, 2020

exterm commented Nov 9, 2020

doodzik left a comment

shageman commented Nov 12, 2020

exterm commented Nov 13, 2020

exterm commented Nov 13, 2020

exterm commented Nov 23, 2020

shageman commented Nov 24, 2020

exterm commented Nov 25, 2020

exterm left a comment

Add to node parsing capabilities #54

Add to node parsing capabilities #54

Conversation

shageman commented Oct 29, 2020

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Checklist

shageman commented Oct 29, 2020

wildmaples commented Oct 29, 2020

doodzik left a comment • edited Loading

Choose a reason for hiding this comment

shageman commented Oct 29, 2020

shageman commented Oct 29, 2020

doodzik Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

shageman Oct 29, 2020

Choose a reason for hiding this comment

shageman commented Oct 29, 2020

exterm commented Oct 30, 2020

shageman commented Oct 30, 2020 • edited Loading

shageman commented Oct 31, 2020

exterm commented Nov 9, 2020

doodzik left a comment

Choose a reason for hiding this comment

shageman commented Nov 12, 2020

exterm commented Nov 13, 2020

exterm commented Nov 13, 2020

exterm commented Nov 23, 2020

shageman commented Nov 24, 2020

exterm commented Nov 25, 2020

exterm left a comment

Choose a reason for hiding this comment

doodzik left a comment •

edited

Loading

doodzik Oct 29, 2020 •

edited

Loading

shageman commented Oct 30, 2020 •

edited

Loading