Fix on save option? What happened to it?

[dsifford]

Perhaps I missed the discussion somewhere, but the editorconfig package has gotten so much less useful for me since fix on save was disabled.

Was there a reason for this that I'm missing?

This wouldn't that big of an issue of there was some visible cue somewhere that told me that the file is not formatted correctly (I thought there was a mouse icon in the lower panel that was supposed to do that? Where'd that go?)

Anyway, consider this a vote for "make format on save" an option again.

Thanks

[florianb]

Hi @dsifford, thanks for getting in touch.

I am not sure what fix on soave you were experiencing. As far as i know atom-editorconfig never did fix anything on save. And it is not intended to do so.

The 🐭 -status-icon appears if there are any issues possibly preventing the package from working in general. So not seeing the mouse is a good thing.

If you would like to fix a file, you can do so by invoking "Editorconfig: Fix File" which currently tries to reset malformed line endings and tab-characters.

Unfortunately it is unlikely we will implement a "on save" triggered autofix due to performance issues.

I hope i could help you with your issue.

[dsifford]

@florianb Thanks for getting back so quickly.

I thought I recalled the package fixing issues on save in the not-too-distant past, but maybe I'm remembering incorrectly.

What motivated me to create the issue is that yesterday I was making some edits to a shell file and even after invoking "fix file", tab spacing wasn't getting fixed.

You can close this issue for now and I'll see if I can maybe get an example of that. Might have just been a fluke too. Who knows...

[sindresorhus]

It didn't fix on save, but it did use to fix on open. Just tried it with an older version, v1.2.4.

[florianb]

That's true, i almost forgot this was implemented. It was removed since it was treated as unexpected behavior that the file is being changed on opening (please see #69). It was one of my first commits here. 🎉

[dsifford]

@florianb It looks like the reporter of that issue simply didn't want files that he/she inspected to be automatically fixed.

Couldn't you just bump the fixing to when a user saves the file to get the best of both worlds? Or at the very least make this optional?

[dsifford]

For those who might also want this to be triggered on save, I added this to my init script and it seems to do the trick.

atom.workspace.observeTextEditors(editor => {
        editor.onDidSave(() => {
            atom.commands.dispatch(atom.views.getView(editor), 'EditorConfig:fix-file');
            setTimeout(() => {
                atom.notifications.getNotifications().forEach(n => {
                    if (n.getType() === 'success') n.dismiss();
                });
                atom.notifications.clear();
            }, 1000);
        });
    });

[florianb]

@dsifford - thank you very much, don't get me wrong: i would appreciate if this plugin would automatically conform the files you're working on. Currently this is not implemented due to a FAQ-entry on Editoroconfig:

Existing files are not reformatted. Only newly input lines are formatted in the format given in the .editorconfig files.

I currently appreciate this because fixing files is an expensive operation and unexpected changes to files are a big source of confusion. In my personal opinion i must say, i would always enforce people to explicitly invoke the fixing of a file.

Introducing the choice of an autofix would need us to implement a configuration-parameter. And even though i am fast with configuration-options in general, this package tries to find wise and expectable defaults which are applicable for all users.

[dsifford]

@florianb Hm, interesting.

Re: the FAQ entry on editorconfig proper.

This is not true for pasted text. There's often times where I'll pull a chunk of code from somewhere and paste it into the editor and in languages where spacing matters (eg yaml, python, stylus, etc) I'll get errors because the pasted in text uses hard tabs vs my editor uses soft tabs.

Adding the hack above to my init script fixes this.

[...] because fixing files is an expensive operation

What would be the difference between me triggering using ctrl+shift+p then selecting EditorConfig: fix-file vs the editor doing that for me? The operation isn't one that is constantly running in the background, correct? The only expense that I'd imagine would be the time it takes to fix a file, no?

With that in mind, would you also say that Atom Beautify is expensive?

I feel like I must be missing some critical implementation detail somewhere.

Thanks for the clarification and your work on this. I appreciate the discussion!

[florianb]

@dsifford - thanks for pointing that out. True - pasted content should be fixed. I did a first attempt while working on #125. Unfortunately i didn't find a way to intercept a paste-event.

I dislike acting onDidSave in a intrusive way. Namely leaving the editor in an "changed" state. I can imagine that a lot of users would complain about this behavior.

I don't want to discuss atom-beatify in this issue, it is just not comparable to the way we're trying to fix things. We are not aware of the grammar and we "just" do stupid replacements which are fast enough.

When i was considering how to fix things and where to let the fixation happen i was aiming for putting it into a onWillSave event.

However, while drafting it i experienced a lot of caveats to consider and one was that operations on onWillSave get very fast expensive from a users perspective. Especially if they're working on large files. Because i want try to make careful progress i decided to put the functionality into a dedicated command.

In fact i'd would like to reconsider putting it into onWillSave again after we made some experience with it and the functionality got mature enough to be automatically applied. Even though this wouldn't be standard conform.

[dsifford]

@florianb Thanks for the clarification. Didn't see the onWillSave event dispatcher in the documentation last night when I was breezing through it. I agree that using that as opposed to onDidSave makes more sense.

Let me know if there's anything I can do to help the cause. I'd be glad to lend a hand on something if you needed more people power.

---

Unrelated, and I can open a new issue for this as well if you'd like: What are your thoughts on removing {dismissable: true} from "success" events? It's somewhat annoying to have to manually dismiss notifications that aren't really providing any useful information other than, "hey, it worked!".

[florianb]

@dsifford thanks for your understanding - i would like to close this issue for now.

I guess before reconsidering working on putting the fixation into onWillSave - we need to fix #168 and i'd like to have some kinds of benchmarks for the fix-file command showing the impact on different file sizes as well as on different amounts of changes.

I would really appreciate any support here and i am very happy about your feedback. Even though i can't immediately take the steps towards your satisfaction you are influencing the direction in a long term.

---

About the dismissable-thing. I do not insist on that. I guess other notifications can't be changed since they show too much information to be dismissed automatically. The fix-file-notification is pretty small and the user should be able to read it in a second.

So if you'd like to change it - i would support it.