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

Some minor usability fixes #321

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

lspitzner
Copy link
Contributor

Thanks for your continued work on ghcid. It is an essential haskell tool.

These are a few random fixes for minor issues. I have put them in four different commits so feel to cherry-pick the ones that you like and leave the rest. I hope it is fine that I use one PR instead of four.

In order of the commits

  • cabal repl did work for me in the source dir of ghcid due to the .ghci file. Not sure what it is supposed to be doing, but I deleted it and it worked. I guess this breaks the tests? I am confused about the setup.
  • document that when you pass a -c --command there are no default --restart conditions any longer.
  • When quitting ghcid, it previously did leave the output file in its current state. If you forget to (re)start ghcid and start working on your code, this can be confusing "still 'all good'? but I inserted a typed hole!" or the worse "I pushed a change because 'all good'" but it was all broken :)
    (N.B. this changes how often options are parsed. This is harmless/an improvement, right?)
  • In a multi-package project, cabal v2-repl likes to cd into the package path, so all paths are relative to some subdirectory. This confuses my downstream tooling, so I added a hack to make the paths absolute (relying on loadFile). Is hacky, but works (and might work for both .hs and .lhs. But not hs-boot, now that I think of it..) There might neater ways of doing a string replacement, but I don't know the CWD so I don't know which path to replace.

@ndmitchell
Copy link
Owner

Thanks for the PR's!. I've merged the doc comment about --command (a very useful clarification), and the printing Ghcid has stopped (good catch, could potentially have been very confusing).

For the .ghci file, that's how I develop Ghcid itself - it lets me load everything into the repl at once - the library and the binary. In contrast, when I try cabal repl with my Cabal v2 I get told that I can't load multiple components. cabal repl lib:ghcid does work for me, even with the .ghci file. It's unfortunate that none of the tooling is very good and there's nothing that works for everything.

For making paths absolute, I don't think that's the right place, and many users will prefer to see relative paths as they are shorter but equally descriptive. Having an --absolute flag wouldn't be unreasonable, but having cabal v2-repl print out relative paths that are wrong just seems buggy - so fixing upstream in Cabal would be ideal. If we did want to do that, the Message type has a loadFile field, which gives the filename as it originally came. Making that name absolute, or adjusting how relative it is, would be a better way to go.

@lspitzner lspitzner force-pushed the master branch 2 times, most recently from 5e74791 to dd4c0a8 Compare July 9, 2020 11:34
@lspitzner
Copy link
Contributor Author

Updated, made the one remaining feature gated behind commandline flag.

For making paths absolute, I don't think that's the right place, and many users will prefer to see relative paths as they are shorter but equally descriptive.

yeah, I agree - the default should be the exact output from ghci.

Having an --absolute flag wouldn't be unreasonable

Done; I have named it --force-absolute-paths, it only affects output-file output.

but having cabal v2-repl print out relative paths that are wrong just seems buggy - so fixing upstream in Cabal would be ideal.

I agree. It is just that contributing to cabal is a much slower and time-consuming process, for some reason. I'd rather get this fixed right now.

(See haskell/cabal#5001 and haskell/cabal#1842 for some relevant tickets.)

If we did want to do that, the Message type has a loadFile field, which gives the filename as it originally came.

I don't entirely follow; my original PR here already relied on the loadFile field. Which is absolute and works great.

Oh, and you were right, specifying the desired component allows using ghcid on ghcid without any problem.

Copy link
Owner

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Cool, adding force absolute paths behind a flag doesn't seem unreasonable. And I can see why its easier to get it into ghcid, so that's not a big deal. I don't quite see how it works though - what if loadMessage was empty? How do you know this isn't losing information or switching file information?

_ | not replacePaths -> l
"" -> l
c : _ | isSpace c -> l
_ -> if "hs:" `isInfixOf` l
Copy link
Owner

Choose a reason for hiding this comment

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

So this looks for Foo.hs: where hs: is the suffix of that string, and replaces it with loadFile? What if you don't have loadFile as the absolute version of this file, but it's a different file? That seems a dangerous operation to do.

@lspitzner
Copy link
Contributor Author

what if loadMessage was empty

I don't understand. Why would loadMessage be empty? And if it was, you'd get empty output (for that Load) just like you did before.

Note that if replacePaths is false the semantics are exactly the same as what currently exists on master. The difference is just that concatMap is expressed as a list-comprehension that supports modifying elements if replacePaths is true.

How do you know this isn't losing information ..

do you mean because the replace might catch more than a file path? yeah, this trusts on the compiler having consistent output.

.. or switching file information?

As I understand, you consider the case where the message mentions a file that is entirely different from the loadFile. Not just absolute vs relative-to-something. You are right, the code does not currently not guard against that. One second.

@lspitzner
Copy link
Contributor Author

so the relative paths are written just like src/Ghcid.hs (and not anything like ./src/Ghcid.hs) which means that a simple suffix check should suffice, i.e. "is the thing being replaced a suffix of the thing I am replacing with". Unfortunately there is a console color escape in front of it..

Apart from the color escape code, would this be an acceptable solution to this? And do you have an idea on how to handle the escape code?

@lspitzner
Copy link
Contributor Author

(I can understand if this workaround is becoming too complex for its own good and you'd rather not merge. I may point out that I have been using this flag without any issue for some weeks now. Maybe just clearly documenting it as an "experimental workaround" would be a compromise? I don't want to spend many more iterations on upstreaming this thing either..)

cabal repl switches current directory for subpackages of
a project, resulting in error paths relative to some
subdirectory. This confuses downstream tooling.

Related cabal tickets are
haskell/cabal#5001
haskell/cabal#1842

Gated behind the newly added flag --force-absolute-paths
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