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

Added format on save feature. #11

Closed
wants to merge 3 commits into from
Closed

Conversation

bcomnes
Copy link
Collaborator

@bcomnes bcomnes commented Mar 16, 2015

Please don't merge yet. A few questions:

  • Is there a way to ask SublimeLint if the active file is javascript? For now, I just reused the is_javascript() function.
  • Performance is really bad at the moment. I had to do naive PATH editing, but I was getting much better performance using subprocess and standard-format. I'm not sure what the culprit is yet. (see https://github.com/bcomnes/sublime-standard-format/blob/master/standard-format.py#L44)
  • How should I allow people to specify which file extensions to include/exclude beyond the defaults?

Other things todo here:

  • Set up command palette option that toggles this on and off.
  • Set the default state to not automatically format on save.
  • Fix the style errors

@bcomnes
Copy link
Collaborator Author

bcomnes commented Apr 1, 2015

@Flet Are you aware of any ways to detect active syntax through Sublime Linter itself? Right now, I have to check if the file is javascript every on_pre_save event, which isn't a huge deal but it would be better if we only listen for on_pre_save for javascript files.

PS Super clean job on auto-format selection!

@bcomnes
Copy link
Collaborator Author

bcomnes commented Apr 1, 2015

SL, for some reason, overwites SublimeLinter-standard.sublime-settings and SublimeLinter-contrib-standard.sublime-settings with its own settings. Are linters not supposed to have their own settings files?

@Flet
Copy link
Owner

Flet commented Apr 1, 2015

Hmm, yeah. in linter.py we pass a list of syntax that are valid for the linter.

I see this in persist.py
https://github.com/SublimeLinter/SublimeLinter3/blob/master/lint/persist.py#L363

def get_syntax(view):
    """Return the view's syntax or the syntax it is mapped to in the "syntax_map" setting."""
    view_syntax = view.settings().get('syntax', '')
    mapped_syntax = ''

    if view_syntax:
        match = SYNTAX_RE.search(view_syntax)

        if match:
            view_syntax = match.group(1).lower()
            mapped_syntax = settings.get('syntax_map', {}).get(view_syntax, '').lower()
        else:
            view_syntax = ''

    return mapped_syntax or view_syntax

So, could use that call or define our own by calling view.settings().get('syntax', '')

@Flet
Copy link
Owner

Flet commented Apr 1, 2015

re: settings, yeah I tihnk it would just be another exposed setting in the SublimeLinter-settings

Rabbit hole is here I believe: http://www.sublimelinter.com/en/latest/settings.html#inline-settings

@Flet
Copy link
Owner

Flet commented Apr 1, 2015

Ah, I think its inline_overrides and defauts stuff.
http://www.sublimelinter.com/en/latest/creating_a_linter.html

may be cool to check out flake8 to see how they do it too.
https://github.com/SublimeLinter/SublimeLinter-flake8/blob/master/linter.py

Now the question is how can these be read by formatter.py :)

Toggle switch is in the command palate and in the 
package settings menu.
@bcomnes
Copy link
Collaborator Author

bcomnes commented Apr 1, 2015

Ok, this is almost ready to go. I'm a bit unhappy with the outcome:

  • SublimeLinter does funning things to settings files for linters. It kept overwriting SublimeLinter-contrib-standard.sublime-settings with its own settings, duplicating everything in SublimeLinter.sublime-settings (https://github.com/SublimeLinter/SublimeLinter3/blob/1e3741fa144ac3348513df17dc9240ed12a07541/sublimelinter.py#L57). I had to create SL-contrib-standard.sublime-settings to avoid this. Unconventional naming, but until we find a better way, thats what is required.
  • Disabling the linter does not disable the auto-formatter. We could tie these together, but then we become more tightly coupled to SublimeLinter, and its generally more random details for users to learn about.
  • Other than a few separate utility functions from SL to automate the path stuff, formatting is pretty much its own module that comes along for the ride with the linter.
  • I like convention (eg, prefixing command pallet commands with the plugin name), but I also don't like the naming conventions that SublimeLinter enforces because to follow that conventions means to have long unwieldy names that overlap with the dozens of SublimeLinter commands. I would very much prefer commands prefixed with standard-format: ... than SublimeLinter[-contrib]-standard
  • We should strive to keep things that don't really need frameworks uncoupled from frameworks.
  • SublimeLinter doesn't actually provide an advantage when it comes to hooking into the pre_save event like I had hoped.

SublimeLinter provides a great API for linters and a handy which utility, but I feel like we are putting too much into sublimelinter-contrib-standard by including standard-format as well.

What do you think about putting the formatting efforts into its own plugin like I started here? https://github.com/bcomnes/sublime-standard-format
The path issues that I ran into before aren't actually that big a deal because its pretty easy to just supplement the search path with common locations like /usr/local/bin. I can add you as a contributor if you are interested.

@Flet
Copy link
Owner

Flet commented Apr 1, 2015

Indeed, I agree it feels like trying to do too much, especially since this is built off of a very structured template provided by SublimeLinter.

I think I owe you some asprin for banging your head against the wall on this stuff today 😬

All your points are completely valid. I'm absolutely fine with splitting formatting out of this plugin.

Once sublime-standard-format is ready, we can remove format from this plugin and add a note to the install.txt pointing users there.

Thanks for digging on this :)

@bcomnes
Copy link
Collaborator Author

bcomnes commented Apr 2, 2015

No headache! Its a good way to learn about Sublime Plugins, something I've wanted to look into for a while. Added you as a contributor to https://github.com/bcomnes/sublime-standard-format

@bcomnes bcomnes closed this Apr 2, 2015
@Flet
Copy link
Owner

Flet commented Apr 2, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants