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

Adjacent search results should be folded together #1

Closed
thumphries opened this Issue Oct 3, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@thumphries
Copy link
Owner

thumphries commented Oct 3, 2017

This is most apparent when searching for a value declaration:

screen shot 2017-10-03 at 9 41 25 pm

The current naive implementation shows separate locations, one for the signature, one for the declaration itself.

Ideally, we'd

  • fold through the list of search results
  • merge all the adjacent SrcSpans
  • pretty-print the combined SrcSpan
  • print all the declarations together, one after the other
@ChShersh

This comment has been minimized.

Copy link
Contributor

ChShersh commented Oct 5, 2017

Hello, @thumphries! I would love to implement this feature during Hacktoberfest if you don't mind :) I found hgrep very useful. And I really want to make it better.

@thumphries

This comment has been minimized.

Copy link
Owner Author

thumphries commented Oct 5, 2017

Hey! That sounds great, please let me know if you have any questions.

@bobbyrauchenberg

This comment has been minimized.

Copy link

bobbyrauchenberg commented Oct 6, 2017

Hey @thumphries @ChShersh i started looking at this also - should i drop it? (not sure what the etiquette is in such situations :) )

@ChShersh

This comment has been minimized.

Copy link
Contributor

ChShersh commented Oct 6, 2017

@bobbyrauchenberg I've already started implementing this feature... But if @thumphries want me to drop it in favor of you I can do this. I had experience when I implemented major feature in some library in Idris programming language and my changes were rejected. I was sad. But after some time I understood my failure. I didn't tell repository author my intention. This video by @ndmitchell helped me to understand how OSS works.
Since then I always discuss everything in issues before I start contribute to project :)

@bobbyrauchenberg

This comment has been minimized.

Copy link

bobbyrauchenberg commented Oct 6, 2017

@ChShersh no worries i'm happy to drop it! i would have said sth when i started looking, but i was initially just having a look through the source

@thumphries

This comment has been minimized.

Copy link
Owner Author

thumphries commented Oct 6, 2017

@ChShersh

This comment has been minimized.

Copy link
Contributor

ChShersh commented Oct 6, 2017

@thumphries Am I right that type signature should be merged with value declaration? In that case it's not trivial to detect reliably using current API which search results to combine. I found two non-trivial cases:

foo, bar :: String
foo = "foo"
bar = "bar"
foo :: String

--- fooo
foo = "fool"

Am I right that such cases should be considered very rare and don't care about them at the moment?

@thumphries

This comment has been minimized.

Copy link
Owner Author

thumphries commented Oct 6, 2017

@ChShersh

This comment has been minimized.

Copy link
Contributor

ChShersh commented Oct 6, 2017

@thumphries Thanks for you clarification! Probably I just didn't understand this issue well enough. So let me try to clarify my intention once more time: am I right that I should merge all results inside one file into one big result? Well, as I understand, type of function should be combined with body of this function. But problems can happen when parts of function body are far from each other.

Consider next example. Running hgrep over my artificial example gives next output:

src/Importify/Cabal/Target.hs:69:1-31


68  -- | Directory name for corresponding target.
69  targetIdDir :: TargetId -> Text

src/Importify/Cabal/Target.hs:(70,1)-(72,64)

70  targetIdDir LibraryId               = "library"
71  targetIdDir (ExecutableId exeName)  = "executable@" <> exeName
72  targetIdDir (BenchmarkId benchName) = "benchmark@"  <> benchName

src/Importify/Cabal/Target.hs:80:1-63


80  targetIdDir (TestSuiteId testName)  = "test-suite@" <> testName

As I see it now after my changes this should look like this:

src/Importify/Cabal/Target.hs:[69:1-31][(70,1)-(72,64)][80:1-63]


68  -- | Directory name for corresponding target.
69  targetIdDir :: TargetId -> Text
70  targetIdDir LibraryId               = "library"
71  targetIdDir (ExecutableId exeName)  = "executable@" <> exeName
72  targetIdDir (BenchmarkId benchName) = "benchmark@"  <> benchName
80  targetIdDir (TestSuiteId testName)  = "test-suite@" <> testName

Can you confirm that I understood issue correctly?

@thumphries

This comment has been minimized.

Copy link
Owner Author

thumphries commented Oct 7, 2017

Only adjacent declarations should be combined. So, for your example, I think it should come out like this:

src/Importify/Cabal/Target.hs:(69, 1)-(72,64)


68  -- | Directory name for corresponding target.
69  targetIdDir :: TargetId -> Text
70  targetIdDir LibraryId               = "library"
71  targetIdDir (ExecutableId exeName)  = "executable@" <> exeName
72  targetIdDir (BenchmarkId benchName) = "benchmark@"  <> benchName

src/Importify/Cabal/Target.hs:80:1-63


80  targetIdDir (TestSuiteId testName)  = "test-suite@" <> testName
  • The first two results were combined because 69 and 70 are right next to one another.
  • The second result is not combined, because of the gap between 72 and 80.
  • Note that the location formatting didn't change, we just constructed a bigger SrcSpan from the two locations

Hope that's a little clearer, sorry for the ambiguity.

@thumphries

This comment has been minimized.

Copy link
Owner Author

thumphries commented Oct 7, 2017

You will find everything you need to inspect and combine the SrcSpan here: https://downloads.haskell.org/~ghc/8.0.2/docs/html/libraries/ghc-8.0.2/SrcLoc.html#g:4

@thumphries thumphries closed this in a9cbe81 Oct 7, 2017

thumphries added a commit that referenced this issue Oct 7, 2017

Merge pull request #36 from ChShersh/master
Merge adjacent SrcSpans (#1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment