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

Is it possible to turn off autoformatting? #300

Closed
joneshf opened this issue Jul 3, 2019 · 14 comments · Fixed by #339
Closed

Is it possible to turn off autoformatting? #300

joneshf opened this issue Jul 3, 2019 · 14 comments · Fixed by #339
Assignees

Comments

@joneshf
Copy link
Member

joneshf commented Jul 3, 2019

It looks like every time spago runs, it reformats the Dhall file. Is there a way to tell spago not to format the files when it runs?

@f-f
Copy link
Member

f-f commented Jul 3, 2019

@joneshf what kind of issue are you having with the autoformatting?

From the Slack channel it looks like the problem is that Spago uses a non-default format command. So would it be enough to document that we autoformat with dhall --ascii format?

@joneshf
Copy link
Member Author

joneshf commented Jul 3, 2019

Nah, the issue isn't the output of formatting. spago always formatting is causing issues with other tooling.

@f-f
Copy link
Member

f-f commented Jul 3, 2019

Could you detail the usecase some more?

@joneshf
Copy link
Member Author

joneshf commented Jul 3, 2019

We use make as the build system because we build more than just PureScript. One of the rules deep in the graph depends on spago.dhall. Whenever we run spago, it seems to change spago.dhall which causes a ton of dependent rules to fire.

I'm not looking to turn off formatting for everyone all the time. But for us, it would be great if we could turn it off when we need to.

@f-f
Copy link
Member

f-f commented Jul 3, 2019

@joneshf right, good point. I believe a good solution here could be to check if the file is formatted (with the library equivalent of dhall format --check, which IIRC was introduced for exactly this usecase) and then only format it if necessary

@joneshf
Copy link
Member Author

joneshf commented Jul 4, 2019

Hmm, I dunno. That might help for that one rule, but it doesn't sound it would fix that one rule. Instead of the rule firing each run, it would fire at most two times each time spago.dhall is unformatted. More to the point, there are other rules that can fire and force everything to be rebuilt the next time around. Checking if a file is formatted first seems like it would address the symptom and not the problem. I guess what I'm saying is that even if you did make the change to check formatting before formatting anyway, we'd still have to work around the fact that spago is formatting at all because we can't get an idempotent build otherwise.

Taking a step back, it's a bit unexpected that spago is formatting the file at all when you run spago install or spago sources or something. From the perspective of not needing to install dhall, it makes sense. But, I dunno how many people expect spago to format a file when installing dependencies. I don't say this to sway your opinion. More to provide an outside perspective.

Again, not looking to change the default for everyone. We've already worked around this issue, so it's not a make or break thing. But, it would simplify our build system if we could pass in a flag instead of altering our rules.

@f-f
Copy link
Member

f-f commented Jul 4, 2019

Yeah, makes sense. Let's add a global --no-config-format flag then (or something similar, name suggestions welcome)

aniketd pushed a commit to aniketd/spago that referenced this issue Jul 5, 2019
- Add `dontFormatConfig` to `GlobalOptions`
- Add condition to `Spago.Dhall.format` with
  `Bool` parameter
aniketd pushed a commit to aniketd/spago that referenced this issue Jul 5, 2019
- Add `dontFormatConfig` to `GlobalOptions`
- Add condition to `Spago.Dhall.writeRawExpr` with `Bool` parameter
aniketd pushed a commit to aniketd/spago that referenced this issue Jul 5, 2019
- Add `dontFormatConfig` to `GlobalOptions`
- Add condition to `Spago.Dhall.writeRawExpr` with `Bool` parameter
@aniketd
Copy link
Contributor

aniketd commented Jul 5, 2019

Opened #302 . Please help with review.

@LiamGoodacre
Copy link
Member

The above PR is merged, should this ticket be closed?

@f-f
Copy link
Member

f-f commented Jul 27, 2019

@LiamGoodacre: upon further investigation I realized that the "autoformatting" is happening because we're trying to migrate old configs without checking if they are using the old schema - this is effectively idempotent except it reformats the file

So I think we should get rid of the flag and just write the file if it's effectively needed (that is, Spago doesn't want to reformat your files, but it will need to in some cases, and we cannot have a flag to shut this behaviour of it's needed to proceed)

However before doing this I'm waiting on #301, as that's shuffling some very relevant code

@LiamGoodacre
Copy link
Member

That makes sense, thanks!

@f-f
Copy link
Member

f-f commented Jul 27, 2019

You're welcome 🙂

I'll fix this before the new release (happening any day now)

@f-f f-f self-assigned this Jul 27, 2019
@joneshf
Copy link
Member Author

joneshf commented Jul 27, 2019

Shouldn't it be possible to read an old version without also writing the file out?

@f-f
Copy link
Member

f-f commented Jul 27, 2019

@joneshf yes, this is what will happen after 1.0: we'll either not change the config format or just read the files in old config formats

However while we are in "pre-1.0" I'd like to migrate config files to the newest version as much as we can, so we can deprecate old config formats (and the migration logic itself) and have our "breaking config changes" not be distruptive for users

I think this is fine because "configuration migration" is a one-off write, so it's not disruptive for build systems etc (the bug here is that we're writing the file every time anyways)

If you're curious about all behaviours that will change in 1.0, see #291 (which already contains "stop migrating configs")

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

Successfully merging a pull request may close this issue.

4 participants