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

User Defined types - inconsistant visualization of tool tips #12412

Closed
slavizh opened this issue Nov 9, 2023 · 9 comments · Fixed by #12654
Closed

User Defined types - inconsistant visualization of tool tips #12412

slavizh opened this issue Nov 9, 2023 · 9 comments · Fixed by #12654
Labels
Needs: Upvote This issue requires more votes to be considered
Milestone

Comments

@slavizh
Copy link
Contributor

slavizh commented Nov 9, 2023

Bicep version
Bicep CLI version 0.23.1 (b02de2d)

Describe the bug
We have the following example for main.bicep

type foo = {
  @description('''Source port ranges.
    Can be a single valid port number, a range in the form of \<start\>-\<end\>, or a * for any ports.
    When a wildcard is used, that needs to be the only value.''')
  sourcePortRanges: string[]
}

param foo1 foo

We intentionally put additional indent on new lines for better readability.
When we have not entered the parameter in bicep parameter's file and we want to see the description and type of the parameter the visualization is the following:

image

if we have entered the parameter and we hover over it visualization is different and not correct:

image

The first visualization is the one that is correct and the second one should be the same.

To Reproduce
Provided

Additional context
Add any other context about the problem here.

@anthony-c-martin
Copy link
Member

The 2nd visualization is being rendered as markdown - because of your indentation, the 2nd & 3rd lines are treated as a code block.

I agree we should fix this to be consistent. In the meantime, if you want a workaround, change:

  @description('''Source port ranges.
    Can be a single valid port number, a range in the form of \<start\>-\<end\>, or a * for any ports.
    When a wildcard is used, that needs to be the only value.''')
  sourcePortRanges: string[]

To:

  @description('''Source port ranges.
Can be a single valid port number, a range in the form of \<start\>-\<end\>, or a * for any ports.
When a wildcard is used, that needs to be the only value.''')
  sourcePortRanges: string[]

@slavizh
Copy link
Contributor Author

slavizh commented Nov 10, 2023

@anthony-c-martin yeah, I am aware of the workaround but we started to do the additional indent for better readability. If we start to revert that we will have worse readability and when you fix it we will have to add the indent again. So I would rather wait for this fix instead of doing a ton of work twice. I would assume is not so hard to fix as you already have the right way to render in code.

@jeskew
Copy link
Contributor

jeskew commented Nov 10, 2023

we started to do the additional indent for better readability.

Sorry for the digression, but should we update multiline strings to allow users to set the left margin (like C# raw string literals do)?

If you're writing a description for a property on a type that's deeply nested, shunting everything over to the file's left margin might look odd:

type myType = {
  some: {
    very: {
      deeply: {
        nested: {
          @description('''A description
that
spans
multiple
lines.''')
          property: string
        }
      }
    }
  }
}

It would be easier to follow the type's structure if Bicep multiline strings worked like C# raw string literals, e.g.:

type myType = {
  some: {
    very: {
      deeply: {
        nested: {
          @description('''A description
            that
            spans
            multiple
            lines.
            ''')
          property: string
        }
      }
    }
  }
}

The description in the second example would today end with \n , so this would be a backwards incompatible change, though I'm not sure how disruptive it would be.

@anthony-c-martin
Copy link
Member

anthony-c-martin commented Nov 10, 2023

Sorry for the digression, but should we update multiline strings to allow users to set the left margin (like C# raw string literals do)?

We discussed this at length when designing multi-line strings, but I can't remember the reason we chose to preserve whitespace accurately.

I do remember there were concerns about copy+pasting source code or anything else that could potentially have its meaning changed unexpectedly - this was before loadTextContent() which I think makes it less of a concern. I can't remember if this was the deciding factor however.

Given that this would be a breaking change, I'm wondering if a go-between solution could be to follow this logic when we render the hovers for descriptions.

@jeskew
Copy link
Contributor

jeskew commented Nov 10, 2023

I do remember there were concerns about copy+pasting source code or anything else that could potentially have its meaning changed unexpectedly - this was before loadTextContent() which I think makes it less of a concern. I can't remember if this was the deciding factor however.

That makes sense, though the user experience for adding documentation to nested properties certainly has room for improvement. I'll comb through some older issues to see if there are alternatives we could look at and bring it up at a team discussion.

@slavizh
Copy link
Contributor Author

slavizh commented Nov 12, 2023

I like @jeskew approach.

@stephaniezyen stephaniezyen added Needs: Upvote This issue requires more votes to be considered and removed Needs: Triage 🔍 labels Nov 15, 2023
@slavizh
Copy link
Contributor Author

slavizh commented Nov 30, 2023

@stephaniezyen any rough time line when it can be fixed? Excluding the things Jonny suggested.

@anthony-c-martin
Copy link
Member

@slavizh PR #12654 out to make the rendering consistent between hovers & completions. See #12654 (comment) if you would like to test this out yourself.

@alex-frankel alex-frankel modified the milestones: Committed Backlog, v0.24 Dec 8, 2023
anthony-c-martin added a commit that referenced this issue Dec 10, 2023
* Embed descriptions as-is without modifications: current logic replaces
`\n` with ` \n`, which leads to rendering quirks.
* Ensure we're emitting a double space + newline after every "block".
* Move shared logic to `MarkdownHelper` class.

Closes #12412 

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/12654)
@slavizh
Copy link
Contributor Author

slavizh commented Dec 12, 2023

@anthony-c-martin thank you very much! I won't be able to test it before release as I am going on vacation but will test it once it is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Upvote This issue requires more votes to be considered
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants