Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Unified comment handling #73

Merged
merged 2 commits into from
Mar 21, 2019
Merged

Unified comment handling #73

merged 2 commits into from
Mar 21, 2019

Conversation

bzz
Copy link
Contributor

@bzz bzz commented Mar 20, 2019

Fixes #69 by making sure all trailing/inner/leading comments are handled on all nodes

@bzz bzz requested a review from dennwc March 20, 2019 13:56
dennwc
dennwc reviewed Mar 20, 2019
View reviewed changes
driver/normalizer/normalizer.go Outdated Show resolved Hide resolved
driver/normalizer/normalizer.go Outdated Show resolved Hide resolved
@bzz
Copy link
Contributor Author

bzz commented Mar 20, 2019

Initial feedback addressed in 972446d

@bzz bzz marked this pull request as ready for review March 20, 2019 15:21
@bzz bzz requested a review from dennwc March 20, 2019 16:38
dennwc
dennwc approved these changes Mar 20, 2019
View reviewed changes
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Looks good! Can you please also link the issue to fix comments later in the driver?

@bzz bzz force-pushed the fix-all-comments-69 branch 2 times, most recently from dafaaa2 to d594b07 Compare March 20, 2019 20:41
@bzz
Copy link
Contributor Author

bzz commented Mar 20, 2019

After more considerations, decided to go back to our initial strategy - only knock off the comments from the node that are transformed to semantic UAST.

@bzz bzz requested a review from dennwc March 20, 2019 20:41
@bzz bzz mentioned this pull request Mar 20, 2019
Open
@bzz
Copy link
Contributor Author

bzz commented Mar 20, 2019

Looks good! Can you please also link the issue to fix comments later in the driver?

Sure, #74

And implementation has been updated to be complete and handle all possible cases, so PTAL @dennwc

dennwc
dennwc approved these changes Mar 20, 2019
View reviewed changes
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Looks good!

Only one question - it doesn't change any of the fixtures. I assume this was a bugfix, so it will be nice to add a fixture that actually shows the bug and fails without this patch, so we can check regressions.

bzz added 2 commits March 20, 2019 22:27
@bzz
sem: refactor all comment handling to 3 rules
a2c2f75 
Each drops off comments from the nodes that are already
mapped to smeantic UAST and thus do not expect to
have such properties

Signed-off-by: Alexander Bezzubov <[email protected]>
@bzz
sem: add new fixture for bblfsh#69
07e7b51 
Signed-off-by: Alexander Bezzubov <[email protected]>
@bzz
Copy link
Contributor Author

bzz commented Mar 20, 2019 •
edited
Loading

Only one question - it doesn't change any of the fixtures. I assume this was a bugfix, so it will be nice to add a fixture that actually shows the bug and fails without this patch, so we can check regressions.

Totally forgot to include the fixture 🤦‍♂️ , thank you for catching! added in 07e7b51

@bzz bzz merged commit 8064736 into bblfsh:master Mar 21, 2019
@bzz bzz deleted the fix-all-comments-69 branch March 21, 2019 16:08
@bzz bzz self-assigned this Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dennwc dennwc dennwc approved these changes

Assignees

@bzz bzz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bzz @dennwc