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

#2794 Initial HIndent support #2896

Closed
wants to merge 21 commits into from
Closed

Conversation

uhbif19
Copy link
Collaborator

@uhbif19 uhbif19 commented May 7, 2022

I am not quite sure if I am using HIndent API correctly, but it does format something.

Currently I do not handle config, cuz not sure where to call getConfig :: IO Config (https://github.com/mihaimaruseac/hindent/blob/master/src/main/Main.hs#L77).

There is some story with [Extension] param. As far as I understand providing Config should be enough. (https://github.com/mihaimaruseac/hindent/blob/master/src/HIndent.hs#L71)

@uhbif19 uhbif19 requested a review from jneira as a code owner May 7, 2022 19:55
@uhbif19 uhbif19 force-pushed the hindent-support branch from 53653bf to 47810e6 Compare May 7, 2022 19:57
@pepeiborra
Copy link
Collaborator

You will need to add the test suite to the GitHub CI test.yml

@pepeiborra
Copy link
Collaborator

Currently I do not handle config, cuz not sure where to call getConfig :: IO Config (https://github.com/mihaimaruseac/hindent/blob/master/src/main/Main.hs#L77).

You can setup a handler for the LSP Initialised event and call it there:

{ pluginNotificationHandlers = mkPluginNotificationHandler LSP.SInitialized $ \_ _ _ _ -> do
env <- LSP.getLspEnv
liftIO $ (cb1 <> cb2) env
}

@michaelpj
Copy link
Collaborator

I think it would also be fine to call it in the handler, that's what the stylish-haskell plugin does, for example.

@uhbif19
Copy link
Collaborator Author

uhbif19 commented May 9, 2022

Looks like some yak-shaving required to make this to build: mihaimaruseac/hindent#583

@uhbif19
Copy link
Collaborator Author

uhbif19 commented May 9, 2022

Looks like getConfig is not exported from hindent. So I can either try to change hindent or copy getConfig with Path.File and bunch of dependencies. Which way is better?

@michaelpj
Copy link
Collaborator

So I can either try to change hindent

That would be ideal. Maybe open an issue and see what the maintainer thinks is the best way of doing it. People are typically happy to be included in HLS, so hopefully you'll get a response :)

@uhbif19
Copy link
Collaborator Author

uhbif19 commented May 9, 2022

Issue created: mihaimaruseac/hindent#585

@michaelpj
Copy link
Collaborator

Looks like upstream isn't very responsive at the minute. Perhaps copying getConfig is the least bad option in the short term?

@uhbif19
Copy link
Collaborator Author

uhbif19 commented Jun 15, 2022

@michaelpj Maybe I will write universal config-looking function working for both HIndent and Stan? Where should I place it?

@michaelpj
Copy link
Collaborator

I don't think that necessarily makes sense since different projects have different config mechanisms. As much as possible we should use upstream.

@uhbif19 uhbif19 force-pushed the hindent-support branch 4 times, most recently from 58657f5 to 4b5b209 Compare June 16, 2022 16:55
@uhbif19
Copy link
Collaborator Author

uhbif19 commented Jun 16, 2022

@michaelpj

getConfig was added and tests use configuration file now.

Where is still git-dependencty in cabal project, cuz build fix is not released yet.

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of comment requests.

cabal.project Outdated Show resolved Hide resolved
Left err -> return $ Left $ responseError $ T.pack $ "hident: " ++ err
Right new -> return $ Right $ J.List [TextEdit range (builderToText new)]

-- Copied from
Copy link
Collaborator

Choose a reason for hiding this comment

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

Link to the ticket and explain that we can remove this once upstream gives us a better way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


-- | Finding files.

-- Copied from HIndent, where it was lifted from Stack.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙈

@michaelpj
Copy link
Collaborator

Sorry, just one last thing I missed before - docs! Can you add this plugin to the "Features" doc and also add a line for it about GHC version support on the GHC version compatibility page.

@uhbif19 uhbif19 enabled auto-merge June 17, 2022 22:13
@uhbif19
Copy link
Collaborator Author

uhbif19 commented Jun 18, 2022

@michaelpj Circleci CI tasks are stalled. Can I do something to fix this?

@michaelpj
Copy link
Collaborator

Not sure tbh!

@pepeiborra
Copy link
Collaborator

@uhbif19 are you able to wrap up this PR?

@pepeiborra pepeiborra added the status: unfinished Status for PRs that have been semi-abandoned label Aug 13, 2022
@uhbif19
Copy link
Collaborator Author

uhbif19 commented Aug 13, 2022

@pepeiborra Will try to fix fails in next couple of days.

@pepeiborra pepeiborra enabled auto-merge (squash) August 16, 2022 07:57
@pepeiborra pepeiborra removed the status: unfinished Status for PRs that have been semi-abandoned label Aug 16, 2022
@uhbif19
Copy link
Collaborator Author

uhbif19 commented Aug 16, 2022

@michaelpj Another strange CI (bug?): "Could not find a usable config.yml because you deleted the CircleCI OAuth app."

Do you know anything about it?

@michaelpj
Copy link
Collaborator

You are oddly cursed, nobody else seems to have trouble with CircleCI 😅

@michaelpj
Copy link
Collaborator

Okay, looks like a genuine failure on stackage-lts16 now.

@uhbif19
Copy link
Collaborator Author

uhbif19 commented Aug 17, 2022

This looks like hindent problem, I created an issue: mihaimaruseac/hindent#589

@uhbif19
Copy link
Collaborator Author

uhbif19 commented Aug 22, 2022

Now all CI jobs are getting stuck.

@pepeiborra
Copy link
Collaborator

@uhbif19 are you able to resolve the conflicts? Hopefully that will unstuck CI

@uhbif19
Copy link
Collaborator Author

uhbif19 commented Jul 25, 2023

Lol, I forgot about this PR. Does anyone need it?

@michaelpj
Copy link
Collaborator

Nobody has asked for it since this PR has been open, so I think it's not that popular and probably not worth it.

@michaelpj michaelpj closed this Jul 25, 2023
auto-merge was automatically disabled July 25, 2023 09:22

Pull request was closed

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.

3 participants