Skip to content

Add support for milestones#62

Merged
HowardWolosky merged 9 commits intomasterfrom
etgottli/milestones
Dec 13, 2018
Merged

Add support for milestones#62
HowardWolosky merged 9 commits intomasterfrom
etgottli/milestones

Conversation

@etgottli
Copy link
Copy Markdown
Contributor

@etgottli etgottli commented Dec 5, 2018

Added functionality for the milestones api

Copy link
Copy Markdown
Contributor

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I've noted some requested changes for you to make.

danbelcher-MSFT
danbelcher-MSFT previously approved these changes Dec 6, 2018
Copy link
Copy Markdown
Contributor

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Good update. Minor issues remaining.

Copy link
Copy Markdown
Contributor

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. Minor changes requested. Will be interested in hearing from @danbelcher-MSFT as well.

$existingMilestone.title | Should be $defaultMilestoneTitle1
}

It "Should have the expected state" {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My nitpick here is that these test names are generic and so don't tell you anything about what is actually being tested. When you run the tests, you should see:

For creating a new milestone:
    ...
    'Should be in the closed state'

Copy link
Copy Markdown
Contributor

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@HowardWolosky
Copy link
Copy Markdown
Contributor

Can you please get this branch up-to-date with what's in master? There are currently conflicts preventing the merge.

@etgottli
Copy link
Copy Markdown
Contributor Author

@HowardWolosky I had merged master into this branch before you made that conflicts comment, and I'm not seeing any conflicts anymore. The only thing blocking the merge now is that "The base branch restricts merging to authorized users" and I'm not authorized. I assume you are able to merge the PR?

@HowardWolosky HowardWolosky merged commit 2bd2447 into master Dec 13, 2018
@HowardWolosky
Copy link
Copy Markdown
Contributor

Thanks -- issue was on my end. GitHub was saying that I couldn't rebase your changes on top of master due to merge conflicts, but it had no problem just squashing your changes and then merging on top of master.

@HowardWolosky HowardWolosky deleted the etgottli/milestones branch April 9, 2019 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants