tylermurry / ado-pr-auto-comment

An easy way to automatically add comments to an Azure DevOps pull request
MIT License
3 stars 4 forks source link

Multiline comments #1

Closed jluttrell closed 3 years ago

jluttrell commented 3 years ago

Hi! First off thanks for creating this task. I just started to use it and it works great.

One thing though is I have a use case for a single multi-line comment, however adding new lines in comments input creates a comment per line (which I see you split on "\n" here).

Is there a workaround or perhaps implement a flag that we can set to indicate whether to treat newlines as a new comment or have everything as a single comment? Or may be easiest to just make comments an array.

i.e.

comments:
  - This is
    my multi line
    comment
  - Second comment
  - ...

I'm happy to open a PR. Thanks!

tylermurry commented 3 years ago

Thanks for the issue, @jluttrell!

Originally, I wanted to do exactly what you described with the array. However, the pipeline syntax limits you to what you can do in the UI which does not have the concept of an "array" or "multi-select".

Open to ideas, but I think the most straight-forward way to achieve this is to introduce a (customizable) newline separator. For example:

- task: pr-auto-comment@1
  inputs:
    accessToken: '$(System.AccessToken)'
    newlineSeparator: "||"
    comments: |
      some||multi-line||comment
      another comment

The benefits to me are that it's easy to implement, visually clear and doesn't try to bend yaml backwards. Thoughts?

I'm also very open to you submitting a PR for this issue!

jluttrell commented 3 years ago

pipeline syntax limits you to what you can do in the UI which does not have the concept of an "array" or "multi-select"

Ah I see, that's a bummer.

Specifying newline separator could work. Only thing is it would most likely require using >- (to strip all new line chars) instead of | for readability if the lines in the single comment are long. For example

inputs:
  newlineSeparator: "||"
  comments: >-
    This first line can get really long and maybe even include a [long link](https://www.myreallylonglink.com/deep/path/with?query=params) and we don't want to put anything after it.||
    This will be the second line.
    And this line would be joined to the line above since the newline above would be stripped.

    This, however, would be a new comment since the double new line char above would result in a new line?

Perhaps defining it as a newCommentSeparator would be better?

inputs:
  newCommentSeparator: "||"
  comments: |
    This first line can get really long and maybe even include a [long link](https://www.myreallylonglink.com/deep/path/with?query=params) and we don't want to put anything after it.
    This will be the second line not needing any special treatment for newline char.
    Again, this line would be 3rd line with no special treatment of newline char.
    ||
    This would be a new comment after the `newlineSeparator`

The default for newCommentSeparator would then be \n which I think would preserve current behavior. Thoughts?

I was using this page for reference to yaml multi line string behavior.

tylermurry commented 3 years ago

@jluttrell That is a fantastic idea! 💯

I would be very open to a PR with those changes. Want to run with that and contribute? Or I would be happy to take care of it if you don’t have time. Up to you!

jluttrell commented 3 years ago

I don't mind taking a stab at it! I'll try opening a PR tomorrow or by end of this week

jluttrell commented 3 years ago

Initial implementation in #2. Obviously feel free to update anything you want. Wasn't quite sure on best description for the docs. Also was thinking newCommentDelimiter may be a better name, but I'm fine with whatever you decide. I didn't run either of the package scripts, figured I'd let you handle that if needed for this PR. Let me know if you'd like me to do any of the changes. Thanks!

tylermurry commented 3 years ago

@jluttrell - The release of 1.1.0 added this functionality. Have you had a chance to give it a try?

tylermurry commented 3 years ago

@jluttrell - I'm going to go ahead and close the issue as the feature has been released for a while. I would love to hear your feedback and if it worked well for you!

jluttrell commented 3 years ago

hey @tylermurry - sorry for going MIA, I actually no longer work at my previous company where I was using this so have not tried it out. Appreciate all your help and responsiveness though! I'm sure they're still using it with great success. Cheers!