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 it possible to search for config without getCurrentDirectory #483

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

Conversation

jhrcek
Copy link

@jhrcek jhrcek commented Jun 30, 2024

Reopening #482 whose branch I deleted by mistake.

Original description:
An attempt to resolve #478

The intention is to make it possible to avoid dependency on getCurrentDirectory (which is problematic in haskell-language-server where multiple things running concurrently might depend on current directory and having to setCurrentDirectory just for stylish-haskell to be able to find the right config might lead to various race conditions).

The way to achieve it is to introduce new datatype ConfigSearchStrategy whose constructor SearchFromDirectory startDir makes it possible to supply a directory explicitly.

See this HLS PR for an example of how this new functionality would be used: haskell/haskell-language-server#4338

@jaspervdj
Copy link
Member

This looks good to me: but I think we should either

  1. Use something like formatWith defaultFormatOptions {formatOptionsSearchStrategy = ...}, and keep format as-is to not break backwards compatibility.

  2. Add an explicit example to the CHANGELOG of how code calling format should be ported.

I don't think many tools are calling stylish-haskell as a library, it's mostly used as an executable, so I think (2) is fine and (1) is probably overkill.

@jhrcek
Copy link
Author

jhrcek commented Jun 30, 2024

I went for option 2 as it was easier. But do let me know if you change your mind and I can implement 1 instead. Intuitively I dislike 1 because (if I understand it right) we'd have to introduce a new type of config type that would have to be called differently than the original Config and which would (for now) just specify the config loading strategy?

Maybe this middle-ground solution would be enough to minimize backward api breakage?

-- keep the original api
format :: Maybe ConfigPath -> Maybe FilePath -> String -> IO (Either String Lines)
format Nothing = formatWith SearchFromCurrentDirectory
format (Just cfgPath) = formatWith (UseConfig (unConfigPath cfgPath))

-- introduce new more general function
formatWith :: ConfigSearchStrategy -> Maybe FilePath -> String -> IO (Either String Lines)
formatWith configSearchStrategy maybeFilePath contents = do
  conf <- loadConfig (makeVerbose True) configSearchStrategy
  pure $ runSteps (configLanguageExtensions conf) maybeFilePath (configSteps conf) $ lines contents

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.

add loadConfig that do not depend on CWD
2 participants