-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add properties to javascript #2004
Add properties to javascript #2004
Conversation
Thank you for your work! However, I don't think that can/want to merge this, at least, as it is now. Explaination.Conditional (ternary) operators are a problem because they look like properties. /(?<!\?\s*)<property>(?=\s*:)/; // doesn't work But we could say: "Every property has to be preceded by either /(?<=[{,]\s*)<property>(?=\s*:)/; // doesn't work That would be the correct approach but won't work because of comments: {
// foo isn't preceded by `,` or `{`
foo: 123
}
return true ? // oh no,
foo : 123; "Can't we try to also match the comments before properties? Does that help?" /(?<=[{,](?:\s|<comment>)*)<property>(?=\s*:)/;
// comment might look like this:
/\/\/.*|\/\*[\s\S]*?\*\//; Unfortunately, no. // the lookbehind will match...
return true ? // ...right here -> { // some text
foo : 123;
Maybe a more real-life example:
return condition ?
// some long, boring,
// and clever explaination
foo : bar; Actually, you could do it by making sure that the comment/ Both conditional (ternary) operators and comments can be dealt with individually but Prism just isn't powerful enough to deal with both of them. The result We can either accept false positives (your pattern) or false negatives. I think that we should discuss which option we want (false positives or false negatives or neither) before merging your PR. Personally, I think that just leaving everything as is the best option to highlight code consistently. It's just too easy to false positives/negatives to happen and you can easily find examples of this in the Prism code itself. /cc @mAAdhaTTah @Golmote |
We should look at what false positives look like in practice before deciding which direction we should take on this one. |
The basic idea of this pattern is that a property is preceded by either a This requires well-formatted code to keep the false positives at a minimum and comments are still our enemies but it works reasonably well. // foo is false positive
return reallyLongVariableName ? "foo and some other stuff" +
foo : bar;
// b is a false negative
const foo = { a: 6, /* some comment */ b: 7 }; @EdieLemoine I'm sorry for rejecting your idea too early. That being said: I will take over from here on if that's ok. A robust implementation of this will be a little more complex and I also want to adjust the Also, we should discuss which kinds of properties we want to include. @EdieLemoine included all possible property kinds but I think that's too much. I.e. variables inside computed properties will also be highlighted in the Simple literals (e.g. Thoughts? |
This seems fair. I'm somewhat of the opinion that adding this generally is a good idea, and that I would prefer false negatives to false positives. We also need tests. |
I just noticed that template strings can't be properties, so I'll add computed properties for now. |
While I was at it, I also implemented a sorter pattern for JS variables/literals and changes to |
Also nice: JS and JSON will now produce about the same result for a valid JSON document; the only difference being |
Closed in favor of #3099. |
Adds .property to Javascript object properties.
Tested here: https://regexr.com/4i9kg