Skip to content

Add option to respect default-extensions from .cabal files#759

Merged
mrkkrp merged 1 commit intomasterfrom
amesgen/issue-517
Aug 12, 2021
Merged

Add option to respect default-extensions from .cabal files#759
mrkkrp merged 1 commit intomasterfrom
amesgen/issue-517

Conversation

@amesgen
Copy link
Copy Markdown
Contributor

@amesgen amesgen commented Aug 11, 2021 •
edited by mrkkrp
Loading

Closes #517
Closes #519

TODO:

  • Allow to explicitly set the path to the cabal file and to the source file (for input via stdin, for example for editor integrations)
  • Respect language (in advance of the GHC2021 language in GHC 9.2)
  • Test if this change significantly decreases performance. When ormolu is called with multiple files, we could cache the parsed .cabal files. EDIT see this comment
  • Add tests (could be further extended)
  • What do we want to do when the same file is part of different components? Right now, some arbitrary component is selected (via Map.union).
  • Make this new functionality discoverable, e.g. in the README.

This PR already revealed one issue, #758.

@amesgen amesgen force-pushed the amesgen/issue-517 branch from f18e064 to c4470a9 Compare August 11, 2021 13:19
@amesgen
Copy link
Copy Markdown
Contributor Author

amesgen commented Aug 11, 2021

Concerning performance: This change is opt-in, so nothing will change in the default configuration. When -e is enabled, a small cost, dependent on the depth of the directory tree and the complexity of the .cabal file, has to be payed. When formatting ormolu, this means (using hyperfine and fd):

 $ hyperfine 'fd -e hs . src -X ormolu --mode check'
Benchmark #1: fd -e hs . src -X ormolu --mode check
  Time (mean ± σ):      1.148 s ±  0.031 s    [User: 1.119 s, System: 0.029 s]
  Range (min … max):    1.099 s …  1.198 s    10 runs

 $ hyperfine 'fd -e hs . src -X ormolu -e --mode check'
Benchmark #1: fd -e hs . src -X ormolu -e --mode check
  Time (mean ± σ):      1.291 s ±  0.032 s    [User: 1.250 s, System: 0.042 s]
  Range (min … max):    1.233 s …  1.323 s    10 runs

Considering that low hanging fruits like parallel formatting would make both cases much faster

 $ hyperfine 'fd -e hs . src -x ormolu --mode check'
Benchmark #1: fd -e hs . src -x ormolu --mode check
  Time (mean ± σ):     712.2 ms ±  40.7 ms    [User: 5.590 s, System: 0.981 s]
  Range (min … max):   662.1 ms … 790.8 ms    10 runs

 $ hyperfine 'fd -e hs . src -x ormolu -e --mode check'
Benchmark #1: fd -e hs . src -x ormolu -e --mode check
  Time (mean ± σ):     837.7 ms ±  20.6 ms    [User: 7.159 s, System: 1.104 s]
  Range (min … max):   810.9 ms … 872.6 ms    10 runs

I think that we don't have to worry too much about a performance regression in this PR.

@amesgen amesgen marked this pull request as ready for review August 11, 2021 15:14
mrkkrp
mrkkrp reviewed Aug 11, 2021
View reviewed changes
mrkkrp
mrkkrp reviewed Aug 12, 2021
View reviewed changes
@amesgen
Copy link
Copy Markdown
Contributor Author

amesgen commented Aug 12, 2021 •
edited
Loading

Updated (01-10-2021): See vyorkin/ormolu.el#14 for a way to use this feature with ormolu.el.

@mrkkrp mrkkrp force-pushed the amesgen/issue-517 branch from 254cf0e to 4889228 Compare August 12, 2021 16:14
@mrkkrp mrkkrp merged commit 59fced3 into master Aug 12, 2021
@mrkkrp mrkkrp deleted the amesgen/issue-517 branch August 12, 2021 16:22
@amesgen amesgen mentioned this pull request Sep 23, 2021
Merged
@georgefst georgefst mentioned this pull request Sep 26, 2021
Open
@vyorkin vyorkin mentioned this pull request Oct 20, 2021
Closed
@amesgen amesgen mentioned this pull request Nov 16, 2021
Merged
@brandonchinn178 brandonchinn178 mentioned this pull request Jan 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrkkrp mrkkrp mrkkrp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Convenient default options Ask ghc/cabal for --ghc-opts

2 participants

@amesgen @mrkkrp