.jsbeautifyrc doesn't support all 3 language preferences

[gvn]

I'm not sure if I'm doing something wrong because using a .jsbeautifyrc is undocumented, but from your source it looks like it should be loaded.

Repro:

• Clone https://github.com/mozilla/webmaker-profile-2
• Open the cloned folder in Atom via File -> Open
• Run Beautify from Packages menu on a JS file that has some extra line breaks added

Expected:

• max_preserve_newlines is set to 1, so extra whitespace beyond 1 empty line should be removed

Actual:

• No change aside from some punctuation shifting (oddly this doesn't occur in Sublime Text, which makes me think jslint_happy is also being ignored).

[karolyi]

It's in the wrong format there.

See this for an example of the right format.

[gvn]

Ah, that is one correct schema, but I'm using the format that allows setting CSS and HTML options in conjunction with the JS ones.

See: https://github.com/einars/js-beautify#css--html

[karolyi]

I see no such format there, but will test it manually soon. If js-beautify works with that config, I'll implement it soon. However, according to the sources, it shouldn't work.

[gvn]

Yeah, it's not well documented unfortunately, but it should work. I can dig around some more to see if I can find better docs! 👍

[karolyi]

I'm sorry to tell you, but I tested it manually, putting the mentioned config file and a test .js file in the same directory. js-beautify, the package behind this one, does not handle that config format.

[karolyi]

Issue closing in 3...2...1... last comments?

[gvn]

Interesting. How do you plan on allowing people to configure their CSS and HTML options?

The format I'm using works with Sublime HTMLPrettify (https://github.com/victorporof/Sublime-HTMLPrettify#using-your-own-jsbeautifyrc-options), so maybe you could accept both schemas to increase interoperability?

If you'd accept that, I can work on a PR...

[karolyi]

Hm, that sublime plugin (I'm a sublime user too) uses its own logic for separating different beautifier configurations in one file. Although it's a logical approach, it's not supported officially by js-beautifier. If you want to do a PR, please create it in a way that the plugin would be able to recognize both format. It's somewhat messy, and you'll probably sweat blood with config-chain (extra logic would need to be added there after parsing).

If so, good luck. If not, I'd create it somewhen, that config-chain stuff is a mess anyway, so I could ditch it.

[donaldpipowitch]

The format I'm using works with Sublime HTMLPrettify

This should probably be something which should be officially supported by js-beautify. Maybe you can create an issue their? // cc @einars

[gvn]

There is actually an open issue upstream (beautifier/js-beautify#391).

It appears that there is an unofficial standard forming around projects wrapping js-beautify supporting all 3 languages preferences in their .jsbeautifyrc.

I agree it would be best to get this unofficial standard officially supported in the core library. In the meantime perhaps at least supporting the JS portion of JSON configs with sub-objects would be good.

[karolyi]

It seems they still didn't make a decision about which way to go there, either to create a new project (beautify-web), or update the functionality with jsbeautify. (I'd take the latter approach).

In the meantime I'm not sure if we should provide this functionality on top of that library.

[donaldpipowitch]

Interesting. Well, I would go the other way and create a new project for that like "beautify-web", because "jsbeautify" is so misleading, if it not just beautifies JavaScript.

On the other hand... if you just want to beautify JavaScript there are other projects which are a little bit more promising: jscs (which will support auto-formatting in the future) or esformatter.

[donaldpipowitch]

@gvn Could you check out the new examples folder? It looks like the nested settings still don't work: https://github.com/donaldpipowitch/atom-beautify/tree/master/examples/nested-jsbeautifyrc
I used "indent_size" with a value of 2 for JS, 4 for CSS and 6 for HTML, but after merging every setting seems to become 2?

[gvn]

Hmm, yeah I can see why. Everything is getting flattened down to a single level and indent_size is the same property name used by all 3 formatters, so they get merged together to a single shared value. There are a few other properties with shared names too, like indent-char.

I'm wondering how this is supposed to be differentiated in the original non-nested schema.

For the nested one I can see about preventing them from being merged together somehow...

[donaldpipowitch]

I didn't looked to much into the SublimeText plugin and how they do that, but I thought that was the purpose of the nested configuration so a user can choose to use different indent_char and indent_size dependent on file type? Isn't that the case?

[gvn]

Honestly I didn't realize you could put all language config at the same level to begin with! I thought you had to nest them. But yeah, that's probably why they did it (also, organization).

I'll see if I can get another PR together soon to enable varying same-name properties between languages.

[karolyi]

Let me add some information here.

When you pushed the PR, I looked into it and already have seen that it would not work. You were listening for nested objects in the configuration, and while the assumption is right, the configuration for each type (html/css/js) can be nested further.

My suggestion would be to check if the parsed json has "html", "js", or "css" keys, and then use the appropriate key.

[gvn]

I have a sense of how to add this functionality, but it may be a week or so until I get a chance to put a patch together...

[donaldpipowitch]

I was a little bit to fast and already merged it with master (normally I'm not the maintainer of a project, but a contributor^^), but it isn't released yet. I think we can wait a week :)

[gvn]

The same issue existed before the patch I believe. Or at least you couldn't
set different spacing by language anyway. Thanks for your patience!

On Wednesday, May 21, 2014, Donald Pipowitch notifications@github.com
wrote:

I was a little bit to fast and already merged it with master (normally I'm
not the maintainer of a project, but a contributor^^), but it isn't
released yet. I think we can wait a week :)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/9#issuecomment-43850283
.

Sent from gPhone

[donaldpipowitch]

Yeah, you're right. You couldn't set a different spacing by language before either :)

[karolyi]

Btw, when I bumped the version number in package.json lately, the package didn't get updated on my atom. Actually, it is still on the one old version before I committed my first PR (2.0.4). apm update does nothing.

Are there any hooks needed to be installed in the repo to get the package synchronized? If yes, are they set up correctly?

[donaldpipowitch]

Yeah, you need to set up apm correctly. I think it was with this command: $ apm dev atom-beautify (see here). Than you can use $ apm publish patch|minor|major to release a new version and increase the version number. You don't need to edit the package.json manually.

But there was a small problem with the version number, because I tried to publish my package even before I had a valid Atom account. (I thought the account was only needed to download Atom and not to publish packages. Yes, I made this package before I had access to Atom itself ;) ) Bummer. I think it was fixed by now.

[Glavin001]

What's the status of this issue? This 58a375f may have resolved it. Let me know.

[karolyi]

#9 (comment) should be implemented IMHO.

[Glavin001]

Can we close this? Or make clear what is TODO? 😄

[karolyi]

I see you're really active, implement the solution in my comment and let this be done then :)

[Glavin001]

I thought I already did? 58a375f

The options file was not working at all for me, so I fixed it a while back. It detects if the configuration file is either nested or simple (detection code was there before) and then merges in the correct respective configurations -- js key for JavaScript, html for HTML, etc.
Is this what UX you intended, @karolyi ?

[Glavin001]

Update:

the configuration for each type (html/css/js) can be nested further.

I have a little change to make 😄.

I prefer your idea over what was there (that checked first level values if Objects):

My suggestion would be to check if the parsed json has "html", "js", or "css" keys, and then use the appropriate key.

Consider it done 👍.

[karolyi]

It's done, thx :)