Autofix replaces leading comments

[silvenon]

My file starts with // @flow, but that comment gets erased with autofix.

I'll gladly fix this, I'm just wondering if it's ok that I completely remove the replacing behavior. I think it was meant to replace wrong headers, but in reality it's replacing any first non-shebang comment, which is not always what we want.

Or maybe we could measure how closely the header is matching given pattern using string-similarity and decide if it's a wrong header or an unrelated comment?

Update

tl;dr for anyone who doesn't feel like reading all of these comments, basically the plugin cannot know whether the existing header comment is an old header that should be replaced (e.g. the year changed) or an unrelated comment. This issue was raised before v3 was released, i.e. before the pattern option, so now we're discussing how we can use it, and ESLint suggestions, to our advantage.

[Stuk]

Hmm, this is tricky... generally the copyright statement should be the first thing in the file. It also appears that the @flow comment does not have to be the first comment. It can even be inside another larger comment.

My preferred solution would be to move the flow comment from the first line. Otherwise this plugin will have to become far more complex, both detecting, and fixing.

What do you think?

[pranaygp]

I agree with @Stuk on this. Facebook themselves have the flow annotations after the copyright comment section.

Alternatively, to ensure 100% flow coverage, just add the @flow to your header config with this plugin 😜

[silvenon]

Exactly like you said, I meant that the // @flow comment should be moved below the injected header instead of being replaced by the header.

What I want is this:

// @flow
console.log(1);

to become this:

// Copyright 2018, My Company
// @flow
console.log(1);

but I would also make sure that this:

// Copyright 2017, My Company
console.log(1);

becomes this:

// Copyright 2018, My Company
console.log(1);

instead of this:

// Copyright 2018, My Company
// Copyright 2017, My Company
console.log(1);

I would achieve this by comparing similarities, it would not specifically look for @flow comments, just any comments which probably wasn't intended to be the header.

If this is something that you want, I'll gladly start working on it.

[pranaygp]

That becomes a harder problem. How would you compare similarities? What's to differentiate @flow from Copyright 2018, My Company in terms of similarity to Copyright 2017, My Company?

The similarity between strings is usually measured by hamming distance, which is pretty expensive to calculate for each header in a large codebase. Besides the time, I think settling on some arbitrary hamming distance threshold to decide what behaviour should occur is odd.

A better solution IMO might be to have a configuration option where you can define your behaviour (think of something like a behaviour flag which can be "replace" or "append"), and after we have that working, extend it so you can define both behaviours, and provide each one with a regex which can be used to decide what behaviour to follow based on the existing header.

I don't think the use case is common enough (and if it come up, can usually be fixed with a one time 'Find and Replace') to warrant adding all that complexity to this plugin

[silvenon]

The similarity between strings is usually measured by hamming distance, which is pretty expensive to calculate for each header in a large codebase. Besides the time, I think settling on some arbitrary hamming distance threshold to decide what behaviour should occur is odd.

Similarity would be calculated only if a file starts with a string that isn't equal to the header, and matching for equality is cheap.

A better solution IMO might be to have a configuration option where you can define your behaviour (think of something like a behaviour flag which can be "replace" or "append"), and after we have that working, extend it so you can define both behaviours, and provide each one with a regex which can be used to decide what behaviour to follow based on the existing header.

Sorry, I lost you here. Could you explain with examples of configuration and result?

I don't see an easy way to solve this problem, all I know is that autofixing this:

// this function does something
function doSomething() { }

into this:

// Copyright 2018, My Company
function doSomething() { }

isn't correct. I'll take some more time to think about it.

[Stuk]

@silvenon ah I see!

I wonder if we can actually special case a few comments. The one that come to mind are @flow, eslint-env *. and eslint-disable .*. If the first comment matches those then we prepend, not replace.

The alternative, more generic solution is not doing anything in "fix" mode when there is a comment at the top of the file. e,g,

console.log(1);

would get the header prepended, but

// anything
console.log(1);

would not. It would then be up to users to manually fix up these cases. We could have an option to turn on "aggressive" mode, if you do want to replace all comments.

[silvenon]

Whitelisting comments is something that crossed my mind as well, but the example in #16 (comment) wouldn't cover it, and it's a common case.

I like the idea of not autofixing if the file starts with a comment. On the other hand, it would be a shame to not be able to update your headers using autofix…

Ugh, complicated stuff. 😜

[silvenon]

@Stuk I see what you meant by identifying special comments and prepending the header in those cases. Is it ok if I proceed with that?

• if a file starts with a @flow/eslint-*/etc. comment, the header will be prepended
• if the file starts with an unrecognized comment, ESLint will only raise an error incorrect header
• if the file doesn't start with a comment, the header will be prepended

The downside is that you would not be able to update your headers using autofix, but often you can do that with a simple search & replace, no?

[pranaygp]

@silvenon

I must say, I don't like the idea of a whitelist of specific comments that warrant prepending the comment. I think it makes this harder to document, and increases the cost to the developer who now has to think about and realise that the plugin does that. I'm not a big fan of increasing developer cost (both in terms of configuration and non-intuitive convention). Anyway, that's just my 2 cents on having a whitelist.

A lot of files will tend to start with unrecognized comments, as coding style at a lot of companies involves a necessary documentation line for the class/function being exported. The only exception here obviously is that you might have import statements at the top.
However, another concern here is a lot of people would like to use this extension to automatically update copyright lines/licensing headers when something changes (think of FB going from BSD + Patents to other licenses recently) or simply when the year rolls over. You could have an 'aggressive' configuration mode (as @Stuk already suggested) for autofix that overrides the behaviour you suggested, but it becomes one more configuration and adds to developer cost. (Not that it's a bad idea, but we should consider this cost vs. the benefits of what you're suggesting).

Anyway, on the second point, I think you've already recognised it too, and so like you said, simply using find and replace should do the trick. Then again, if that's the case, this plugin feels to me like it effectively only becomes useful for a one time use. Someone can set it up, run eslint --fix once, and then the autofix practically never becomes useful again after that.

If all of this is only because the autofix removes @flow comments at the top, I honestly thing that's something that will get caught when making a PR that introduces the headers generated by this plugin. Simple fix would be to go to those files, and re-add @flow. It's a one time fix one the user's side that's hard to miss and very intuitive. No added developer cost thereafter from this relatively simple plugin

[silvenon]

The user wouldn't have to think about a whitelist, I think the plugin itself would have it predefined.

Concerning the rest of what you said, sure, the user could manually add the deleted comments back, but:

1. the deletion itself is very unexpected, the user might not notice it right away

2. if it happens across many files (like it happened to me yesterday), adding them back manually is not a viable option

3. adding them back automatically (i.e. reverting) is hard if you have other changes as well, it's not a simple git checkout

4. you will have to add deleted comments back every time you update your header/header rule with a different header, for example:

   // Copyright 2018
   
   // @flow
   import React from 'react';

   after changing the year, the above will be autofixed into:

   // Copyright 2019
   import React from 'react';

Many files start with comments (either Flow, JSDoc or something else), so this behavior could use some rethinking. What do you think?

[pranaygp]

Hmm, I guess the whitelist could work. I'm not a big fan, but it might be the best solution and atleast better than always deleting @flow comments.

Just to throw other ideas out there, what do you think about supporting something like this; Headers generated by autofix will be at the top of the file, and will end with something like this.

/**
 * ...
 * END ESLINT PLUGIN HEADER
/*

It will always be prepended to the top of the file, and will never replace existing comments. Also, if the header is changed in the future from configuration, the plugin only has the authority to change things before that line.

So:

// @flow
import React from 'react';

becomes

/**
 * Copyright 2018
 * END ESLINT-PLUGIN-HEADER
 * The above header is autogenerated and should not be manually modified.
 */

// @flow
import React from 'react';

and

/**
 * Copyright 2016
 * END ESLINT-PLUGIN-HEADER
 * The above header is autogenerated and should not be manually modified.
 */

// @flow
import React from 'react';

becomes this

/**
 * Copyright 2018
 * END ESLINT-PLUGIN-HEADER
 * The above header is autogenerated and should not be manually modified.
 */

// @flow
import React from 'react';

[silvenon]

Yep, that could work! It's not ideal that the comment depends on the technology used, but it seems to be the most stable option so far.

[xuqingkuang]

I got the same issue, but my codes are starts with /* eslint-disable xxx */ to disable some rules, but the plugin remove them all with fix option.

[CoderCoco]

I'd like to also add that we've started using this in our TypeScript project and files without any imports are having their leading doc comments stripped.

So say I have this file for the header:

/*!
 * put this in the file
 */

and this code in the file

/**
 * This interface does something and should be used with {@link MyInterface2}
 */
export interface Test {
  // Interface code
}

Running the fixer will generate this output.

/*!
 * put this in the file
 */
export interface Test {
  // Interface code
}

In the above scenario, I would not expect the comment to be dropped. We use the /*! for our license file so that we can easily differentiate them from a /** comment. Perhaps if we could provide a custom matcher to the plugin that looks for a /*! all /** would be ignored and left untouched.

[Stuk]

It looks like the new eslint suggestions feature would help with this. When a leading comment is found we could suggest either adding the header before the existing comment or replacing it.

[silvenon]

That's great! It would be nice to be able to run eslint --fix for all files to automatically update all headers, but this plugin can't know for sure what the person wants, so it would be better to use suggestions instead. Even with the new pattern option, we might have entirely changed our header, so the plugin can't know what the old pattern was, the only thing it knows for sure is that the required header is not there. Changing from old header to new is a job for codemods.

[silvenon]

Actually, it can autofix in the case that the header comment matches pattern, otherwise it should use suggestions.

I noticed that currently it's not possible to configure the rule to retrieve the header comment from a file and set the pattern, correct? Could we use this opportunity to change the API to allow this?

[maksnester]

Sorry for offtop, but I just wanted to say that one more nice thing that would be great to have is a blank line after the generated header.