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

Reference inline comments. #2677

Merged
merged 1 commit into from
Sep 9, 2015
Merged

Reference inline comments. #2677

merged 1 commit into from
Sep 9, 2015

Conversation

betaorbust
Copy link
Contributor

This pull fixes #2675.

As described in the issue, in-value comments are not currently preserved in when referenced. This patch adds reference marking to nodes below rules and expressions if markReferenced is available.

No changes in benchmark runtime outside of test noise levels.

@seven-phases-max
Copy link
Member

This looks fine for me. Though to be honest I wonder if we really need these three separate functions for relatively auxiliary stuff inside already pretty overloaded Rule class. Can't we have all the code in one function instead? (feel free to accept my changes or leave your code as is - I'm just never happy with whatever code).

Also it would be nice to have a dedicated test for the "array" branch of the markReference just to easily detect when it's broken... (For the moment I can't quickly find some simple Less snippet when this code is reached at all :(

As described in less#2675 in-value comments are not preserved in referenced rules.

This patch adds reference marking to nodes below rules and expressions if markReferenced is available.
@betaorbust
Copy link
Contributor Author

The code to get to the array path that I ran into was:

.selector{
  prop: value /*comment*/;
}

In this case, the value becomes an array of nodes instead of a single node.
I reworked the code a bit to take your suggestion into consideration. I had to change the format to be a bit more verbose because of the eslint rules, but the idea is still the same.
The reason it's a declared function is because markReferenced might get called thousands of times, and having an anonymous function inside markReferenced would generate a function object thousands of times -- basically this just helps it stay fast so my patch has the smallest perf impact possible on the LESS compilation time.

@seven-phases-max
Copy link
Member

and having an anonymous function inside markReferenced would generate a function object thousands of times

This makes sense (though I'm always assuming V8 which is fine with (not anonymous) functions if their code does not use any outer stuff).

lukeapage added a commit that referenced this pull request Sep 9, 2015
@lukeapage lukeapage merged commit 3ef676a into less:master Sep 9, 2015
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.

Comments not preserved in referenced includes
3 participants