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

Added structs to supports potential actions #103

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

nmaupu
Copy link

@nmaupu nmaupu commented Apr 26, 2021

@atc0005 atc0005 self-requested a review April 27, 2021 10:27
@atc0005
Copy link
Owner

atc0005 commented Apr 27, 2021

Thanks for submitting this.

It will take me some time to step through the changes and offer detailed feedback, but I wanted to go ahead and throw out some unordered items as they come to mind:

@atc0005
Copy link
Owner

atc0005 commented May 16, 2021

@nmaupu I meant to follow-up before now, apologies for the silence here.

Do you happen to have any test/prototype client code which uses the functionality added by this PR? I'm asking before I make plans to try and work in changes to a small project of my own in order to better test the proposed changes.

@nmaupu
Copy link
Author

nmaupu commented May 20, 2021

@nmaupu I meant to follow-up before now, apologies for the silence here.

No prob!

Do you happen to have any test/prototype client code which uses the functionality added by this PR? I'm asking before I make plans to try and work in changes to a small project of my own in order to better test the proposed changes.

No I don't ATM :/

@atc0005
Copy link
Owner

atc0005 commented Jun 18, 2021

I'm coming at this with fresh eyes and haven't gone back over the documentation yet; what is the name parameter used for?

I posted this question in-line here:
nmaupu@b9e05e8#diff-6e76fa32b560c5449670f8c369fbd67f095af1b2b108e65da37dab3d55a7dddaR554

I'm used to GitHub displaying the applicable portion of the diff alongside comments within the PR "thread". Not sure why it didn't do so this time.

@atc0005
Copy link
Owner

atc0005 commented Jun 24, 2021

@nmaupu

Thanks for your patience on this.

I've gotten far enough with a set of changes for a CLI client that I'm getting a good feel for the features this PR adds. Overall, this appears to be a great addition to the library.

Are you interested in making further changes, or would you like to smooth off any rough edges (e.g., add doc comments in a few places) and consider this PR done? If you'd like to make further changes, I've got some suggestions, but I can hold those for a follow-up PR if you want.

atc0005 added a commit to atc0005/send2teams that referenced this pull request Jun 24, 2021
This commit implements support for specifying multiple URLs
via CLI flags. These URLs are exposed as labelled "buttons"
towards the bottom of the generated Microsoft Teams message.

This commit also includes modified, vendored WIP changes
from atc0005/go-teams-notify#103 that have yet to be
merged into that project. The intent is to update to
the latest stable tag once that PR and any follow-up
PRs are merged.

Light documentation updates have been applied, including usage
of the new `--target-url` flag.

refs atc0005/go-teams-notify#103
refs #148
refs #120
@atc0005 atc0005 added this to the Next Release milestone Jul 2, 2021
@atc0005 atc0005 added the enhancement New feature or request label Jul 2, 2021
@atc0005
Copy link
Owner

atc0005 commented Jul 2, 2021

@nmaupu I initially tried to use GitHub to rebase the latest changes from master, but they were merged in as new commits instead of fast-forwarding the branch. I ended up dropping that attempt, then rebased and force-pushed.

The result is that the branch is up to date with master and the intermediate commits that GitHub created have been dropped. This wasn't as clean as I had hoped, but your branch is rebased and represents where it would be if you had only just opened the PR based on current master.

Copy link
Owner

@atc0005 atc0005 left a comment

Choose a reason for hiding this comment

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

Going to approve the PR as-is, then submit further changes (e.g., linting fixes) as part a later PR. See #102 (comment) and #102 (comment) for specifics.

Thanks again for this work, it's a great addition to the library.

@atc0005 atc0005 merged commit ab452a2 into atc0005:master Jul 2, 2021
@atc0005 atc0005 linked an issue Jul 2, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for PotentialActions
2 participants