sindresorhus / atom-editorconfig

Helps developers maintain consistent coding styles between different editors
https://atom.io/packages/editorconfig
MIT License
812 stars 80 forks source link

Fix on save #173

Closed corysimmons closed 1 year ago

corysimmons commented 7 years ago

It seems like Fix already exists. It'd be nice to have a setting for fix on save. Appreciate your work and feel free to ignore this. Just thought it'd be a nice feature.

dmytrokyrychuk commented 7 years ago

You don't have to wait for the package to implement this feature.

Add the following to your atom init script (menu: Edit > Init Script...):

applyEditorconfigFixOnSave = () ->

  addOnSaveCallbackToEditor = (editor) ->
    editor.onDidSave ->
      atom.commands.dispatch(atom.views.getView(atom.workspace), 'EditorConfig:fix-file')

  atom.workspace.observeTextEditors(addOnSaveCallbackToEditor)

applyEditorconfigFixOnSave()

If you wish to disable this later, you'll have to remove the code from the init script, and then to restart Atom (window reload should be enough too).

corysimmons commented 7 years ago

Great solution, and it taught me how to add inits to Atom. Thanks!

Leaving this open just in case Sindres decides he wants it.

corysimmons commented 7 years ago

Oh, there does seem to be an inconvenience with the workaround. I keep getting these popups. Happen to know how to remove them?

image

dmytrokyrychuk commented 7 years ago

Seems like popups get displayed unconditionally: https://github.com/sindresorhus/atom-editorconfig/blob/master/commands/fix.js#L93

I may have a workaround for this too :) You can prevent popups from being displayed by commenting (or removing) the line 93 in the commands/fix.js file. You should be able to find it here: ~/.atom/packages/editorconfig/commands/fix.js. Note that you'll have to reapply the workaround after the package get upgraded.

dmytrokyrychuk commented 7 years ago

The proper way to fix this would be to make a PR that adds an option to hide the popups.

corysimmons commented 7 years ago

You're a champion. Thank you. It's working great. Feels good to save a file and it just autocleans everything without bothering me.

I almost feel like this behavior should be the default as it'd help people adhere to editorconfig.

sindresorhus commented 7 years ago

Sure. Fix on Save sounds like a useful option. Pull request welcome. (See https://github.com/sindresorhus/atom-autoprefixer/commit/cc3e2308c6f3aa85b8935a1ecf3f325c8fbdaef4 on how to implement it).

florianb commented 7 years ago

That's a great improvement - i want to bring to your attention that relying on onWillSave is a performance risk on big files (and was already discussed in other issues like #166).

I don't like relying on onDidSave because it will leave a changed file in an unsaved state. This may be treated as unexpected behavior. I did reject adding a second save-state because of the implications caused by calling the save-event immediately after another.

I would really appreciate adding this functionality to atom-editorconfig. And i would prefer having it acting within onWillSave. However, i won't merge a PR in which isn't backed by performance tests for large files (probably @sindresorhus does).

I guess there is room for improvement in the fix-command. I just hadn't had the time to dig into it, yet.

corysimmons commented 7 years ago

I can confirm it is slow on larger files if they aren't conforming to editorconfig already (seems to be fine if there are only a few changes to make on save, but slow when there are thousands). Might be Atom slowness though.

florianb commented 7 years ago

I have spent some more thought on this.

I guess it should be possible to performantly check large files for malformed line endings or indent styles. In that case we could show the mouse, alerting the user that she saved a malformed file. Offering a button to fix the file alongside with the warning.

This should lower the risk that the user is keeping malformed files by accident.

What do you think?

dmytrokyrychuk commented 7 years ago

Seems like the linter package will have an API that might be sufficient for this. See the description of the Linter Message v2 type: https://github.com/steelbrain/linter/blob/master/docs/types/linter-message-v2.md. It will accept an array of solutions that either contain a text to replace, or a callback that fixes an issue. Once linter v2 is released, editorconfig could use the callback version of solution to apply indentation and line-ending fixes.

florianb commented 7 years ago

Though i like the idea to use nifty features f.e. from the linter-package i don't think this would be applicable for editorconfig. Because it is unlikely that a malformed character comes alone. And as far as i know we would need to provide a single solution/fix per malformed range of characters. It would need multiple interactions of the user to fix f.e. multiple lines of wrong indentation characters.

I could imagine showing the 🐭 where a click on it shows the status notification which then contains a button to fix the file at once.

dmytrokyrychuk commented 7 years ago

And as far as i know we would need to provide a single solution/fix per malformed range of characters.

Technically, it's a solution per message. You can push a single message per file to linter, whose apply fixes indentation of the whole file at once.

florianb commented 7 years ago

That would be considerable. But it still adds a dependency.

dmytrokyrychuk commented 7 years ago

It's not a direct dependency. If the linter provider is not consumed by other packages (the linter package would normally consume them), this feature would just be inactive.

Most Atom users probably have linter installed anyway. It might be worth mentioning about it in the readme, so that other people can install it if they'd like to be notified when their files are not conform to editorconfig.

florianb commented 7 years ago

I would prefer if our users might be able to use all features without relying on third party packages.

And i would say it is even more likely that people do not use the linter package than f.e. disable the status-bar package. But as i had to learn recently, even the status-bar can't be expected.

What are the arguments against an implementation in the existing notification-system?

sindresorhus commented 1 year ago

Closing as Atom is abandoned.