Skip to content

Conversation

@german1608
Copy link
Collaborator

Summary

This PR adds jump-to-definition on let bindings. It is really similar to haddock's:

  • If you hover a variable, all references of that variable will be highlighted including its definition
  • Each variable use is an anchor HTML tag to the declaration of the variable in the source code.

I also updated dhall-docs-gen.nix with the latest changes from dhall-lang's Prelude

Solution

Check the header of Dhall.Docs.CodeRenderer for a high-level explanation of how the code is rendered.

To discuss

I left some FIXMEs comments on the code about how we handle labels that are quoted using backticks. To solve those I was thinking we could do the following:

  • Use bindingSrc0 and bindingSrc1 to generate the actual label.
  • Parse the actual parsed label by changing the Binding data type to something like this:
    data Binding = { ... value :: (Src, Text), ... }
    
    ...although it will introduce a breaking change and I will need to change all value references (PTSD of feat(comments): support prefix comments on Record's key-value pairs. #1908)

Let me know if you have any idea or if you like any of my above proposals.

@german1608
Copy link
Collaborator Author

@sjakobi I tried your suggestion of splitting out the pure part of renderWithHyperLinks from the HTML generation section but the code ended up more complicated; probably it's because it's 1 AM right here and I'm sleepy ;) tomorrow I can give it another try

@sjakobi
Copy link
Collaborator

sjakobi commented Aug 3, 2020

@german1608 Where do I find examples of this feature in action?

When I try opening the golden HTML files in my browser I only get a blank page.

I tried your suggestion of splitting out the pure part of renderWithHyperLinks from the HTML generation section but the code ended up more complicated

No worries. It was just an idea – it's not a priority.

@german1608
Copy link
Collaborator Author

@german1608 Where do I find examples of this feature in action?

When I try opening the golden HTML files in my browser I only get a blank page.

Check the generated docs on hydra for Prelude. The golden files can't be displayed on the browser because of a bug in the HTML prettyprinter

@sjakobi
Copy link
Collaborator

sjakobi commented Aug 3, 2020

Check the generated docs on hydra for Prelude.

Do you mean https://hydra.dhall-lang.org/build/68681/download/1/docs/? I can't find any docs that look changed.

The golden files can't be displayed on the browser because of a bug in the HTML prettyprinter

That needs to be fixed. Can you record an issue? We could even consider removing the pretty-printing step.

@german1608
Copy link
Collaborator Author

Do you mean https://hydra.dhall-lang.org/build/68681/download/1/docs/? I can't find any docs that look changed.

Check here for example: https://hydra.dhall-lang.org/build/68681/download/1/docs/Map/keys.dhall.html

The golden files can't be displayed on the browser because of a bug in the HTML prettyprinter

That needs to be fixed. Can you record an issue? We could even consider removing the pretty-printing step.

Sure. The pretty-printing step it's nice to have a better diff view when there is a failure.

@german1608
Copy link
Collaborator Author

@sjakobi I've just noticed that I forgot to push the update for prelude docs, the link we had earlier is outdated so you should refresh it

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Jump-to-definition works really nicely in https://hydra.dhall-lang.org/build/68717/download/1/docs/JSON/renderAs.dhall.html. 💪

I think the feature is a bit hard to discover though – it would be nice if declarations and uses would be highlighted a bit.

I don't currently understand the problem with backticked variables. Why does the way they appear in the source code matter? Modulo shadowing, all you need to match a variable to its binding is the text in Var.


BTW, did you get variable shadowing right? E.g.

let a = 1

let a = 2

in a

Here the last a should only reference the a = 2 binding.

A test would be good to have for this.

And what about

let a = 1

let a = 2

in a@1

?

@sjakobi
Copy link
Collaborator

sjakobi commented Aug 4, 2020

Sorry for the late review BTW. I'm pretty busy with non-OSS things this week. I hope I'll have more time next week.

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

LGTM! I'd still like to see some highlighting of the linked variables, but it's not urgent.

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

Very nice job! 🙂

@german1608
Copy link
Collaborator Author

LGTM! I'd still like to see some highlighting of the linked variables, but it's not urgent.

@sjakobi: the variable highlights when you put the mouse over it (hover it). Do you mean to highlight it in some background all the time?

Now when the user clicks on a variable declared on a let-binding
it will jump to that location. If it hovers it, it will highlights
all the usages.
@german1608 german1608 force-pushed the feat/jump-to-let-bindings branch from cdeb756 to 2f19677 Compare August 4, 2020 15:41
@sjakobi
Copy link
Collaborator

sjakobi commented Aug 4, 2020

LGTM! I'd still like to see some highlighting of the linked variables, but it's not urgent.

@sjakobi: the variable highlights when you put the mouse over it (hover it). Do you mean to highlight it in some background all the time?

Yeah. I thought it would be nice if those parts of the code that are not just boring text would be highlighted a bit. Currently, I think this feature is a bit hard to discover.

@mergify mergify bot merged commit b0c0987 into master Aug 4, 2020
@mergify mergify bot deleted the feat/jump-to-let-bindings branch August 4, 2020 16:00
@german1608
Copy link
Collaborator Author

Yeah. I thought it would be nice if those parts of the code that are not just boring text would be highlighted a bit. Currently, I think this feature is a bit hard to discover.

Ok! I'll do some UI/UX fixing in a further PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants