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

Make indentation configurable via --tabsize; closes #210 #289

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

specious
Copy link

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 specious mentioned this pull request Jan 15, 2017
@bgourlie
Copy link

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

@bgourlie
Copy link

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

@rtfeldman
Copy link

rtfeldman commented Jan 15, 2017

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. 😬

@scarfacedeb
Copy link

@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.

@specious
Copy link
Author

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

@scarfacedeb
Copy link

@specious ok, no problem!

@tibastral
Copy link

Thank you for this PR, it's awesome

@mattjbray
Copy link
Contributor

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.)

@tibastral
Copy link

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 ?

@geraldoquagliato
Copy link

"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.

@specious
Copy link
Author

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

@fiatjaf
Copy link

fiatjaf commented Oct 15, 2017

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

@specious
Copy link
Author

I merged in changes from origin/master at d8e6435: b0d99a5

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.

@supermario
Copy link
Contributor

supermario commented Dec 2, 2017

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!

@bgourlie
Copy link

bgourlie commented Dec 2, 2017

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

@ghost
Copy link

ghost commented Apr 15, 2018

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.

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.

9 participants