Navigation Menu

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

Adding option to print node value #2008

Merged
merged 1 commit into from Oct 11, 2018
Merged

Conversation

jlgonzalezdev
Copy link
Contributor

@jlgonzalezdev jlgonzalezdev commented Oct 5, 2018

Basically I did some tweaks to the jsx-no-literals rule to be able to print the node value. :)

Fixes #2007.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Can you elaborate on your use case here? It seems like you're trying to parse eslint rule messages, which is absolutely not a supported thing anywhere in the eslint ecosystem, and not safe to rely on (they can change in any version, not just majors).

@@ -25,23 +25,29 @@ module.exports = {
properties: {
noStrings: {
type: 'boolean'
},
printNodeValue: {
Copy link
Member

Choose a reason for hiding this comment

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

i do not think it makes any sense to add this as a configuration option; if it's useful, it should be in the message for everyone.

@jlgonzalezdev
Copy link
Contributor Author

Yes, Basically My use case is:
I need to print to the report the hardcoded text found in the JSX templates.
for something like SIGN IN
I need an output like:
Missing JSX expression container around literal string: SIGN IN.

So, I need to print the node value("SIGN IN " in the example above).

I think that context.getSourceCode() instead of node.value would be the right approach for this, I'll be making some changes to my fork.

@ljharb
Copy link
Member

ljharb commented Oct 5, 2018

@jlgonzalezdev let's remove the option and just do this by default; i don't see any reason not to.

@jlgonzalezdev
Copy link
Contributor Author

@ljharb ok, removing the option

@@ -41,7 +42,7 @@ module.exports = {
function reportLiteralNode(node) {
context.report({
node: node,
message: message
message: `${message}:${sourceCode.getText(node).trim()}`
Copy link
Member

Choose a reason for hiding this comment

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

Let’s add a space after the colon, and it’d be good to have some kind of boundary markers around the text - backticks or curly quotes, maybe?

Separately, I’m not sure if the trimming is a good idea - if it’s a literal, the whitespace could be important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s add a space after the colon, and it’d be good to have some kind of boundary markers around the text - backticks or curly quotes, maybe?

Sure, let's use it like this-> : "TEXT"

Separately, I’m not sure if the trimming is a good idea - if it’s a literal, the whitespace could be important.

I think Not having the trimming really impact the readability of the output, in every case that I have work with this kind of output, I have needed to use the trimming. So, I woud like to keep it.

@ljharb ljharb force-pushed the master branch 2 times, most recently from e415132 to eafd19d Compare October 11, 2018 04:02
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM

This was referenced Jan 4, 2019
@ghost ghost mentioned this pull request Jan 12, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants