Skip to content

Conversation

mrkkrp
Copy link
Member

@mrkkrp mrkkrp commented Jan 17, 2023

No description provided.

@mrkkrp mrkkrp requested a review from amesgen January 17, 2023 11:05
@mrkkrp mrkkrp force-pushed the export-getFixityOverridesForSourceFile branch from ede4626 to a47fae1 Compare January 17, 2023 11:05
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A thought: Do we maybe only want to export a more "coarse" way how to "mirror" the behavior of the ormolu executable by e.g. providing a function

configForFile :: FilePath -> IO (Config RegionIndices)

(maybe also including some kind of verbosity argument that e.g. controls whether we log when no .cabal file could be found) that we also use in app/Main.hs? That way, it would be impossible to "forget" to expose new functionality in Ormolu, and users don't have to manually assemble the Config beforehand.

Comment on lines +6 to +11
* Exported `getFixityOverridesForSourceFile` from the `Ormolu` module making
it part of the stable API. [PR
970](https://github.com/tweag/ormolu/pull/970).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the rule "Only the Ormolu module is public API" codified anywhere? (I am actually not sure where I got this from.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. There has been very few guarantees about stability of the API. We did not even annotate any functions with @since, it was always rather "use it if you'd like, but it is not as stable as a normal library would be". I think we should indeed make it more explicit what modules are supposed to be stable.

@mrkkrp mrkkrp force-pushed the export-getFixityOverridesForSourceFile branch from a47fae1 to 72f973c Compare January 17, 2023 20:15
@mrkkrp
Copy link
Member Author

mrkkrp commented Jan 17, 2023

Re configForFile, I think we never really heard from HLS plugin writers or other people who use Ormolu as a library, so there were just my vague suppositions about what blocks could be potentially useful. I always assumed that it should be possible to achieve pretty much everything, and if someone is not happy we'd hear from them and adjust accordingly. The reason there was no simple function that gives the user a ready to use Config RegionIndices is because the process of its construction is very much connected to all the various command line options that Ormolu CLI can receive, so if it were to be general and to protect from things being forgotten, the main function would need to use configForFile too and therefore it would need to accept many of the various Opts things which may be relevant for a particular third party use case or they might be irrelevant and just annoying. I am not sure where the demarcation line between bundling too much of Ormolu CLI logic and ease of use lies...

@amesgen
Copy link
Member

amesgen commented Jan 17, 2023

Yeah, it seems sensible to spend some time at this point to properly define the Haskell Ormolu API:

  • Who are the intended users of the API? HLS is one currently. Is there any other conceivable non-internal user of the Ormolu Haskell API? Does Ormolu Live count? There, we use various stuff not in Ormolu.
  • How easy should it be to mirror the default Ormolu executable invocation? With the API exposed via 0.5.1.0, it is not possible to do so only via the Ormolu module. Also, it seems conceptually neat to let app/Main.hs to be not much more than a optparse-applicative parser for the Config and maybe a FileContext type.

@mrkkrp
Copy link
Member Author

mrkkrp commented Jan 18, 2023

Sounds good, I will rework this PR to:

  • Define and explicitly state what parts of the packages are considered "stable".
  • Provide a "neat" way to do all the configuration and deduction work so that it can be combined with the existing ormolu-like functions to pretty much achieve what main currently does.

@mrkkrp mrkkrp closed this in #976 Jan 25, 2023
@mrkkrp mrkkrp deleted the export-getFixityOverridesForSourceFile branch February 25, 2023 20:52
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.

2 participants