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

Should you be able to test empty strings with ESLintTester? #546

Closed
nzakas opened this Issue Jan 20, 2014 · 17 comments

Comments

Projects
None yet
3 participants
@nzakas
Copy link
Member

nzakas commented Jan 20, 2014

Based on a discussion in #529.

There was some confusion over whether or not we need to allow for empty-string code tests in ESLintTester. Right now, ESLintTester does not allow it:
https://github.com/eslint/eslint/blob/master/lib/tests/eslintTester.js#L68

The question is, do we need to allow testing of empty string or not? If yes, why?

(Arguably, ESLint should just not execute on an empty string.)

@nzakas nzakas referenced this issue Jan 20, 2014

Closed

Fix #528 #529

@dmp42

This comment has been minimized.

Copy link
Contributor

dmp42 commented Jan 20, 2014

Indeed ESLint might not execute anything on an empty string. I guess it's a choice. Now, the question remains: how are we gonna test that? I don't think that's good to just say "we do nothing on empty string, so, we don't need to test that fact".

On the other hand, I don't quite understand why getSource would return null (instead of the empty string) when currentText == "": https://github.com/eslint/eslint/blob/master/lib/eslint.js#L416 - though this is apparently expected that way: https://github.com/eslint/eslint/blob/master/tests/lib/eslint.js#L515

As for a quick patch preventing eslint.verify to evaluate rules on empty content, that may be enough:

https://github.com/eslint/eslint/blob/master/lib/eslint.js#L260

  • if (!parseError) {
  • if (!parseError && text) {

but I don't feel comfy enough yet with eslint source as a whole to judge on that.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 20, 2014

The test would be at the eslint.js level in this case. If fed an empty string, no events should fire, no warnings should be produced, and no errors should be thrown.

Returning null from getSource() was supposed to be in the case that no file has been loaded yet. The side effect is that it also happens when an empty string is fed in.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 20, 2014

@nzakas:

If fed an empty string, no events should fire, no warnings should be produced, and no errors should be thrown.

Not true. The empty program parses as a Program node ({type: "Program", body: []}), upon which many rules act. Also, as @dmp42 hints at, it's appropriate in many cases to make sure a rule doesn't (or maybe does) alert on an empty program.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 20, 2014

If that's not true then it's a bug.

My point is, it shouldn't be up to the rule to make sure it works with an
empty program. If there's no text, then no rules should be run and ESLint
should exit. Why force each rule to handle this special case when the rule
can't actually do anything?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 20, 2014

Here's my rule, no-empty-programs:

module.exports = function(context){
  return {
    Program: function(node){
      if(0 === node.body.length) context.report(node, "empty program detected");
    }
  };
};

The same could be done for even-length series of statements or any number of predicates that the empty program would violate.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 20, 2014

I'm having a hard time understanding if you're describing a hypothetical
use case or one you actually have. Of course, it possible to write a rule
like that, but I don't see finding empty files as fitting in with the goals
of ESLint (which is to tell you about your code - of which there is none in
this case).

@dmp42

This comment has been minimized.

Copy link
Contributor

dmp42 commented Jan 22, 2014

Ok... what about first solving this question: does it make sense for getSource() to return null if it was fed an empty string?
What's your thoughts on this?

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 22, 2014

I don't believe that's relevant to the issue at hand. If rules should never
be called for an empty string, then getSource() will never be called and so
its return value doesn't value.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 22, 2014

@nzakas: Where does that assumption come from? Rules can absolutely trigger on empty programs. Remember, as I've been trying to show, the empty program parses to a non-empty AST. That node can (and does!) have rules that trigger on nodes of that type.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 22, 2014

Again, just because rules can be run on empty programs right now doesn't
mean that they should. If we short circuit this operation, then none of
the rules need to specifically test for empty string input. The fact that
we are forcing rules to do that right now is terrible, in my opinion,
because it pushes out a responsibility to every rule that could be
centralized, therefore eliminating a source of error.

As far as I can tell, the only benefit to allowing empty string input to go
through all the rules is that you could potentially flag empty
files...however, as I've said before, I don't believe that should be a goal
of ESLint. The goal is to evaluate your code. If there is no code, ESLint
has no job to do.

I don't know any other code quality tool that does anything with empty
files other than ignoring them, why should ESLint be different?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jan 22, 2014

The fact that we are forcing rules to do that right now

What do you mean by this?

@dmp42

This comment has been minimized.

Copy link
Contributor

dmp42 commented Jan 22, 2014

I don't believe that's relevant to the issue at hand. If rules should never
be called for an empty string, then getSource() will never be called and so
its return value doesn't value.

I must disagree: getSource is part of eslint public API as far as I can tell, and as such, ought to be clear on what values are returned in what conditions. Unless eslint doesn't want to expose getSource as part of its API, then you are not to say what methods people are going to call at what time or under what input conditions.
The very reason this discussion started is because getSource behavior is wonky. I can sure open another ticket if you prefer to keep things separate, but IMHO getSource behavior right now is the real/main problem ;).

Also, rules have a need to special case the empty string input because of getSource behavior. Fix that behavior, and the "problem" goes away - arguing that the "problem" is in allowing rules processing on empty string is kind of disingenuous given that fact, I think...

Best regards.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 22, 2014

@michaelficarra I mean that we are forcing individual rules to include an empty string test for code. That seems silly.

@dmp42 What you're talking about is the expression of a top-level design decision, where getSource() becomes the linchpin in that decision. It shouldn't matter if getSource() returns an empty string or null if rules don't ever get called in that case. Using getSource() at a top level is a rare use case. The goal of returning null is to indicate that whatever you're trying to do won't work. We can have a larger discussion about whether that's correct or not once the top-level decision is finalized.

And let's keep words like "disingenuous" out of discussions. My genuineness should not be on the table for a discussion about implementation.

@dmp42

This comment has been minimized.

Copy link
Contributor

dmp42 commented Jan 22, 2014

The goal of returning null is to indicate that whatever you're trying to do won't work.

And indicating that "things won't work" because eslint was used on an empty file doesn't make much sense from the user POV if you ask...

It shouldn't matter if getSource() returns an empty string or null

Again, it does matter if this is a public API. If it's not, I'll stop arguing.

Using getSource() at a top level is a rare use case

If this is public API, it doesn't matter what we think is rare or not.

And let's keep words like "disingenuous" out of discussions.

Sorry if that was offensive - I'm pretty sure you noticed I'm not a native.

My genuineness

That's not about you, and no one questions your integrity - still, the argument you made is disingenuous, as there would be nothing special to do in the rules to special case empty string input if that weren't for a side-effect of a design decision in getSource.

Best.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 22, 2014

Okay, I think we are arguing about two different things. You are focusing
on the implementation of getSource(). I have no problem changing it to
return an empty string for an empty file. Feel free to submit that as a
pull request.

The point I am arguing about is the higher-level decision of if rules
should or should not be executed for an empty file. My belief is that rules
should not be executed for an empty file. There is no reason to do this,
and I don't believe it's ESLint's job to inform the user that a file is
empty.

@dmp42

This comment has been minimized.

Copy link
Contributor

dmp42 commented Jan 31, 2014

Sorry for the late answer.
That does make sense.
Opening #570 and hopefully pushing something later on.
Thanks!

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Jan 31, 2015

Closing this out. When ESLint receives an empty string, it will simply return an empty array and avoid parsing or running rules.

@eslint eslint locked and limited conversation to collaborators Jan 31, 2015

@nzakas nzakas closed this in 920e191 Jan 31, 2015

nzakas added a commit that referenced this issue Jan 31, 2015

Merge pull request #1754 from eslint/issue546
Update: Fast-path for empty input (fixes #546)

Holixus pushed a commit to Holixus/eslint that referenced this issue Feb 12, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.