Make processString honor exclude field in config

[BujiasBitwise]

Atom's atom-beautify is using the method processString to call csscomb. This is a problem, because processString is not checking the exclude field in the config. That means that packages like atom-beautify, that have the option to call csscomb on save, or init scripts to call csscomb every time you write ";", are beautifying files that are in the exclude list. It has some nasty effects, like reordering mixins and functions, potentially breaking them or other problems.

I thought I would submit a pull request instead of opening a new issue, just in case this simple change is accepted.
I don't know if shouldProcessFile was not being called here by design, or if the parameter filename should be renamed to path. Tell me if there is anything that should be changed.

If it's accepted and the npm package is updated, I'll submit another patch to atom so it can use the new feature, since it's not using filename at the moment.

[BujiasBitwise]

Should I close this and make another pull request to dev branch instead?

[tonyganch]

Are we talking about this package?
https://github.com/bruce/atom-csscomb/

[BujiasBitwise]

We are not talking about atom-csscomb, but that package has the same problem.
We are talking about the package atom-beautify (this is the line where atom-beautify is calling processString), that has the npm csscomb as a dependency.

This pull request is for core, a component of npm's csscomb and a dependency of atom-beautify. Another pull request would have to be sent to atom-beautify, to make it call processString with the options.filename parameter.

[BujiasBitwise]

I'll try to explain this again, because maybe my previous posts were confusing.

The atom packages atom-csscomb and atom-beautify are ignoring the exclude field in .csscomb.json
This is a problem, because for example atom-beautify can be set to beautify on save, and that can break mixins, since you can't add them to the exclude field. Regardless of that, the exclude field should work, it's what any user would expect.

Those 2 atom packages call processString without the filename parameter.
processString is in the npm package csscomb-core, which is a dependency of the npm package csscomb, which is a dependency of the 2 atom packages.

processString is only using the filename parameter to print an error message. This pull request makes processString use that paremeter to check if the file being processed is in the exclude list.

After this, other 2 pull requests would have to be sent to atom-beautify and atom-csscomb, to make them use the filename paremeter.

This is a potential solution, and I would like to know if you want to do it, do something else, or just accept that atom packages should ignore the exclude field.

[tonyganch]

Hi @killerjk! Sorry for a late response.
When making those methods I assumed that a developer uses processPath if he has a file path and wants to check whether it should be processed or not, and processString if he has a buffer with no file path yet.
If we add the path check to processString method, this check will be called several times if you use processPath method because processPath calls both the check and processString.
Do you think the plugins should better use processPath or processFile if they have a file path?

[BujiasBitwise]

I've been testing processFile instead of processString.
I had already tried something similar when I installed csscomb in gulp instead of the editor. The result is... problematic.
Atom detects the change and reloads the file, as expected. Sometimes it's completely seamless, but other times packages like pigments have to parse the file again, so you see the colors reloading, the linter doing its thing, etc. It's like if you were constantly closing and opening the file. Considering the frequency I call csscomb (every save and every ; I type), it's really annoying.

Another problem is packages that depend on files being modified only through the editor may break. For example, local-history will save revisions of the files before they get modified by csscomb. It's not a huge problem, you can always run csscomb again after you restore a revision, but other packages may stop working completely. It's difficult to predict, and it's a valid concern that a piece of software may need to receive a processed string, while still representing a file.

Another thing that makes me worried is the interaction with atom-beautify. It has the option to call csscomb on save, and this is already a problem, sometimes leaving you with an empty file. It's explained here. I don't know what would happen if csscomb modified the file bypassing everything. He says things like "atom and the package have a very special relationship", "everything is async" and "it has to workaround atom in some cases", which makes me thing it can be a can of worms. I suppose he would have a better answer than me, but even if it worked, you still have other packages constantly reloading when using processFile, making processString work much better.

If processFile calls processString, then this patch is clearly not the way to go. I understand the reasons behind this design, processString deals with.. well, strings. They should always be processed. Something else has already done the check, and filename simply represents the file the string may belong to, for debugging purposes.
The problem is this assumes everything that deals with files, is going to modify them directly. We now seem to have an use case where this shouldn't be happening.

It would be nice if you could:
-Process a file (it must use the exclude field)
-Process a file that is in memory as a string (but it's still a file on disk, and should follow use the exclude field). This can potentially be needed by programs that for whatever reason sit between csscomb and the files themselves. For example, imagine a virtual filesystem, or an environment where there are files but you can't normally access them as regular files, you have to go through other protocol.
-Process a string (the exclude field is not used since it doesn't represent any files, even if the string comes from one).

Ways to cleanly design an API that does this? You probably have a better answer than me. You could for example stop calling processString with a filename parameter from processFile, and create another try-catch block there to add filename if processString throws an exception. This way, if processString receives a filename paremeter, it's because it's a file in memory, and the exclude field should be used.

Other way would be making a processStringFile method that atom plugins should call. It would check the exclude field and call processString.

Yet another way would be adding a boolean or something to processString, but that is ugly.

[tonyganch]

@killerjk, thank you very much for taking time to explain it all.
What if shouldProcess(path) method was public?
You can then call it inside plugins like:

var shouldProcess = csscomb.shouldProcess(filename);
if (shouldProcess) {
  csscomb.processString(...);
}

The method which is now "private": https://github.com/csscomb/core/blob/dev/src/core.js#L371

[BujiasBitwise]

That could work, yes. The potential problem is any other person that tries to call csscomb from other program, like a package, will have to know it must be implemented that way, or a part of csscomb's configuration will be ignored without warning. It took me a while to know what was going on, I actually thought it was my fault when writing the wildcards. So we may see more broken packages in the future.

I suppose it would be reasonable to ask plugin's authors to implement more code if they want to have more control. The question is, should csscomb keep the exclude check as something internal, so it always happens regardless of how you call it, or should the author of the external program be tasked with the re-implementation of what would be a custom processFile? Answer that the way you prefer for your project, and we'll make the patches. Just keep in mind we'll probably see more packages that ignore the exclude field if people don't know what they have to do. It's not something immediately obvious.

My opinion is that it sounds like the proper solution. Sure, you could add the check to processString the way I explained before, but imagine what would happen if a package only wanted to process a few strings. It would still have to check the exclude list, and it would do it with every string. I don't know how expensive that would be, but it seems wrong. That check belongs to the code that calls processString, and in this case, it's the plugin. My only concern is that people have no way of knowing they should be doing that to make it work like the user expects unless it's documented.

By the way, I was wrong about pigments, it also reloads with processString. This doesn't change anything though.

[tonyganch]

@killerjk, okay. So I'll move the check to public methods and add documentation for plugin writers mentioning that if they use processString they should also use the check method. Does it sound fine?
I'm stuck with some other issue right now and I'll get to this one once I fix a few tests.

[BujiasBitwise]

That sounds great. Thanks!