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

Add ability to override esformatter options #7

Merged
merged 6 commits into from May 11, 2014

Conversation

brettlangdon
Copy link
Contributor

Mostly I was looking for a way to format to 4 spaces and not 2. Originally I added a cli option to set the number of spaces to indent, but I think this might be a better option?

Also I wasn't sure how I should document an example (if you feel one is necessary).

@jimfleming
Copy link
Contributor

Hey, thanks for taking the time to look into this!

Just thinking out loud but how do you feel about an rc kind of config (e.g. .jsfmtrc)? A quick search found this which might be an easy foot in the door: https://github.com/dominictarr/rc

An advantage of that package may be that it supports --config file out of the box.

I think the benefit of something like an rc config is that you don't need to specify it each time you run the command.

@brettlangdon
Copy link
Contributor Author

That is the first thing I looked for when I wanted to configure it. I think that is a great idea!

@james2doyle
Copy link

I'm wrapping this library in a grunt task, so having as many options exposed as possible is nice!

Also it seems like only rewrite and search are exposed via require. It would be nice to be able to pass an object for the output/config.

Maybe something like jsfmt.rewrite(fileString, replaceRule, { list: false, comments: false })

@jimfleming
Copy link
Contributor

@james2doyle yeah, that would be good. Filed here: #8

EDIT: I'll add the rc on Monday if nobody gets to it before me. I can start exposing more options over the next week.

@jimfleming jimfleming changed the title add ability to override esformatter options Add ability to override esformatter options May 11, 2014
@brettlangdon
Copy link
Contributor Author

@jimfleming I replaced what I had with rc not sure if you wanted to expose options other than the esformatter style settings?

@james2doyle
Copy link

Just had another idea, you could probably piggy back off .jshintrc for the tab sizes, or even .editorconfig.

I just don't know about adding yet another dotfile to projects. Or at least supporting the existing ones might be nice.

@brettlangdon
Copy link
Contributor Author

@james2doyle I added support for picking up indent level from .jshintrc.

.editorconfig could probably be added fairly easily with: https://github.com/editorconfig/editorconfig-core-js.

What would the order of precedence be for this configuration files? Right now I have:
default indent < .jshintrc indent < .jsfmtrc indent

But where would .editorconfig fit in? before .jshintrc?

@james2doyle
Copy link

I would say that jsfmtrc should be swapped with jshintrc. Since it was
added specially for the library. I would make editor config last then.

I don't know how many people use it (editorconfig) but the whole idea of it
is to make the code styles consistent across a project. Maybe it's worth
knowing if anyone uses it before going to the trouble of adding it

On Sunday, May 11, 2014, Brett Langdon notifications@github.com wrote:

@james2doyle https://github.com/james2doyle I added support for picking
up indent level from .jshintrc.

.editorconfig could probably be added fairly easily with:
https://github.com/editorconfig/editorconfig-core-js.

What would the order of precedence be for this configuration files? Right
now I have:
default indent < .jshintrc indent < .jsfmtrc indent

But where would .editorconfig fit in? before .jshintrc?


Reply to this email directly or view it on GitHubhttps://github.com//pull/7#issuecomment-42768432
.

James Doyle

t: @james2doyle https://twitter.com/james2doylew: ohdoylerules.com
http://ohdoylerules.com

@brettlangdon
Copy link
Contributor Author

Oh, I think I may have diagrammed it not the best, .jsfmtrc values are
never overridden by others.

I have it load the default, then try to override with .jshintrc and then
finally override from .jsfmtrc.

Does this still sound correct to you?

I think that's a good idea. I hadn't heard of editorconfig myself until
this morning.
On May 11, 2014 7:58 AM, "James Doyle" notifications@github.com wrote:

I would say that jsfmtrc should be swapped with jshintrc. Since it was
added specially for the library. I would make editor config last then.

I don't know how many people use it (editorconfig) but the whole idea of
it
is to make the code styles consistent across a project. Maybe it's worth
knowing if anyone uses it before going to the trouble of adding it

On Sunday, May 11, 2014, Brett Langdon notifications@github.com wrote:

@james2doyle https://github.com/james2doyle I added support for
picking
up indent level from .jshintrc.

.editorconfig could probably be added fairly easily with:
https://github.com/editorconfig/editorconfig-core-js.

What would the order of precedence be for this configuration files?
Right
now I have:
default indent < .jshintrc indent < .jsfmtrc indent

But where would .editorconfig fit in? before .jshintrc?


Reply to this email directly or view it on GitHub<
https://github.com/rdio/jsfmt/pull/7#issuecomment-42768432>
.

James Doyle

t: @james2doyle https://twitter.com/james2doylew: ohdoylerules.com
http://ohdoylerules.com


Reply to this email directly or view it on GitHubhttps://github.com//pull/7#issuecomment-42768740
.

@jimfleming
Copy link
Contributor

I agree with @james2doyle about waiting to see if there's a large demand for editorconfig (or others) support. It makes sense, I think, but from skimming the comments it also requires another dependency which I think we should avoid this early in development (unless there's more request for support).

I'm a little worried about where it should stop: Sublime, Vim, Emacs et al support formatting configuration. Documenting the diagram of what overrides what could confuse users and supporting conversions between them (as in the jshint indent conversion) could become unwieldy.

@james2doyle
Copy link

Most likely if someone is using .editorconfig, they are using .jshintrc as well, it says something about their personality haha

@jimfleming
Copy link
Contributor

So I just noticed esformatter supports its own rc file. Should we import that since its a direct dependency anyway? Otherwise I think this PR is good to go.

@brettlangdon
Copy link
Contributor Author

From an initial glance at the source it looks like esformatter only
supports the .esformatter file when using it via CLI.
On May 11, 2014 2:46 PM, "Jim Fleming" notifications@github.com wrote:

So I just noticed esformatter supports its own rc file. Should we import
that since its a direct dependency anyway? Otherwise I think this PR is
good to go.


Reply to this email directly or view it on GitHubhttps://github.com//pull/7#issuecomment-42779202
.

@jimfleming
Copy link
Contributor

Ah, good catch, nevermind! I'll merge this in. EDIT: Tested it, seems to work well.

@brettlangdon
Copy link
Contributor Author

Thanks!
On May 11, 2014 3:07 PM, "Jim Fleming" notifications@github.com wrote:

Ah, good catch, nevermind! I'll merge this in.


Reply to this email directly or view it on GitHubhttps://github.com//pull/7#issuecomment-42779748
.

jimfleming added a commit that referenced this pull request May 11, 2014
Add ability to override esformatter options
@jimfleming jimfleming merged commit 0c1e0af into rdio:master May 11, 2014
@jimfleming
Copy link
Contributor

No, thank you!

@brettlangdon brettlangdon deleted the override-options branch May 11, 2014 19:20
@schickling
Copy link

I'd love to see editorconfig support!

@chpio
Copy link

chpio commented Mar 10, 2015

hi,

+1 for editorconfig support, but also indent_style = tab

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

Successfully merging this pull request may close these issues.

None yet

5 participants