elm-format icon indicating copy to clipboard operation
elm-format copied to clipboard

Make indentation configurable via --tabsize; closes #210

Open specious opened this issue 7 years ago • 16 comments

The number of spaces that constitute a tab of indentation in the output code is now configurable via the --tabsize command line argument. Changing the value from a constant to a configurable parameter meant that I had to thread it all the way from the entry point into every function that is affected by the tab size.

Now that it's done, making another deeply permeating value configurable will simply mean converting the tabSize parameter to a record containing more than one member.

This is my first time touching this code, so please give it a careful look over.

specious avatar Jan 15 '17 01:01 specious

You'll need to update the readme, removing pretty much the entire first section, starting with "The benefits of elm-format..."

bgourlie avatar Jan 15 '17 08:01 bgourlie

Or determine new bullet points for that section that would be relevant with a configurable formatter.

bgourlie avatar Jan 15 '17 08:01 bgourlie

I'll open a pull request for review and I understand that you guys may decide not to merge it.

I appreciate the attitude! ❤️

To summarize #210 from my perspective: I challenged anyone who thought this was a good idea to address its drawbacks, but only one person did, and that person's conclusion was that configuration wasn't the way to go after all.

tl;dr since after 44 comments, no advocate for this has been willing or able to defend it from even the very first criticism it received, it seems extraordinarily clear that this should not be merged. 😬

rtfeldman avatar Jan 15 '17 09:01 rtfeldman

@specious you're awesome! 👍

Would it be possible for you to keep it in sync with the upstream repository?

It would be much easier than updating the hardcoded value by hand on every release.

scarfacedeb avatar Jan 19 '17 16:01 scarfacedeb

That might prove to be difficult. The diff for this change is pretty significant.

specious avatar Jan 20 '17 16:01 specious

@specious ok, no problem!

scarfacedeb avatar Jan 21 '17 15:01 scarfacedeb

Thank you for this PR, it's awesome

tibastral avatar Feb 06 '17 09:02 tibastral

I have no authority on the matter, but I suspect this PR won't be accepted.

Perhaps a happy middle ground would be to submit a PR with the changes required to make maintaining a fork with configurable indentation easy? (I.e. this PR, but without the actual --tabsize argument.)

mattjbray avatar Feb 06 '17 21:02 mattjbray

And a .elm-format at the root of the project with : tabsize: 2 on top ?

So we can still use elm format inside atom with existing projects with 4 spaces ?

tibastral avatar Sep 07 '17 21:09 tibastral

"saves time" here means "does the personal preference of the package maintainer".

That would be great if the package maintainer and other famous Elm developers wouldn't be discouraging people from using forks.

I'm waiting for someone able to understand the code for this package (I don't) to create an elm-format-configurable or something like that so I can use it and configure it the way I like.

Even if it has a single configuration option: tabsize, it would already be great.

geraldoquagliato avatar Oct 14 '17 23:10 geraldoquagliato

@geraldoquagliato, try this fork. Build it from source and use --tabsize.

specious avatar Oct 15 '17 21:10 specious

I want to try that, but it is 244 commits behind! Do you know what are we missing? (Nothing important, I imagine?)

fiatjaf avatar Oct 15 '17 22:10 fiatjaf

I merged in changes from origin/master at d8e6435d20500e92bd6152aec81eaef5a2ab25eb: b0d99a5c3eca3833932472ed65793f2dcb27e157

The unit and integration tests are passing and everything looked good to me after I ran it on a few Elm files.

I would appreciate any and all feedback from any other pair of eyes, if you'd take a careful look at my changes.

@avh4, I'll be so grateful if you could shed some light on the significance of the 4 in src/ElmFormat/Render/ElmStructure.hs.

specious avatar Nov 17 '17 14:11 specious

It seemed really crazy to me that we can land 🚀 backwards, and build responsive websites, but we still can't resolve the 2 vs 4 space problem in 2017...!

So I wondered why we can't just visually adjust spacing indentation to what we want, without changing the underlying spaces (like proponents of tabbed-indent argue).

Here's a POC solution for Atom: https://github.com/supermario/visual-indent

I have dropped my custom 2-space elm-format, and been using this method for ~2 months.

In practice, I've completely forgotten there are 4 physical spaces under the hood. All I see is 2 visual spaces when I code.

I get to program in my preference of 2 spaces, and I'm not messing up other folks' OSS work with my custom space sizing 🎉 maybe this will work for some of you as well. 🍰 + eat it too!

supermario avatar Dec 02 '17 12:12 supermario

@supermario What an awesome idea! Looking forward to seeing how this turns out.

bgourlie avatar Dec 02 '17 20:12 bgourlie

It seems reasonable to use tabs instead of spaces if it's not configurable, great example would be how it's made in gofmt. Why not take a great idea from the inspiration? Everyone should move to tabs once. End of the debate.

ghost avatar Apr 15 '18 01:04 ghost