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

Adding functionality for Project Columns #162

Merged
merged 21 commits into from
May 12, 2020

Conversation

jpomfret
Copy link
Contributor

@jpomfret jpomfret commented May 4, 2020

Covers:

  • List project columns
  • Get a project column
  • Create a project column
  • Update a project column
  • Delete a project column
  • Move a project column

The tests don't run successfully without the Project code from PR #160, if you want I can rebase this branch with master once/if the core project code gets merged in?

Copy link
Member

@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.

Once again, this is awesome. Thanks for the contribution. There are a few changes in here that I'd like you to make before we merge this in (after we merge in the project core work).

Additionally, I'd encourage you to think up some suggested Project usage examples that could be added to USAGE.md.

GitHubProjectColumns.ps1 Outdated Show resolved Hide resolved
GitHubProjectColumns.ps1 Outdated Show resolved Hide resolved
GitHubProjectColumns.ps1 Show resolved Hide resolved
Moves the project column with id 999999 to the Last position.

.EXAMPLE
Move-GitHubProjectColumn -Column 999999 -Position After:888888
Copy link
Member

@HowardWolosky HowardWolosky May 6, 2020

Choose a reason for hiding this comment

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

This syntax should be abstracted away from users.

I see two options here (maybe you see more):

  • Have three mutually exclusive parameters: [switch] $First, [switch] $Last, [int64] $After, and add some error checking into the body to ensure that you either have $true for $First or $Last (since a user could pass in -First:$false), or a positive value for $After. Then build the appropriate body value from that.
  • Have a ValidateSet with the options of First, Last, After, and then an additional parameter of [int64] $After and in-body validation that $After is only supplied if ('after' -eq $Position).

I think the first option is cleaner. I'm open to other suggestions. I just don't like users having to come up with the After:888888 syntax themselves. #Closed

Copy link
Contributor Author

@jpomfret jpomfret May 7, 2020

Choose a reason for hiding this comment

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

Totally agree. Thanks for the feedback, went ahead with option one. #Closed

Copy link
Contributor Author

@jpomfret jpomfret May 7, 2020

Choose a reason for hiding this comment

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

Also - wondering if I should be clicking the resolve button when I fix it? Or should that be for you to resolve, once you review the adds?

Sorry if I'm doing this wrong! #Closed

@jpomfret jpomfret mentioned this pull request May 7, 2020
@jpomfret jpomfret requested a review from HowardWolosky May 8, 2020 14:19
Copy link
Member

@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.

Looking good. Just one minor change and should be good to go.

GitHubProjectColumns.ps1 Outdated Show resolved Hide resolved
@HowardWolosky HowardWolosky self-assigned this May 11, 2020
@HowardWolosky HowardWolosky added api completeness This is basic API functionality that hasn't been implemented yet. api-project Work to complete the API's defined here: https://developer.github.com/v3/projects labels May 11, 2020
@jpomfret jpomfret requested a review from HowardWolosky May 11, 2020 07:19
HowardWolosky
HowardWolosky previously approved these changes May 11, 2020
Copy link
Member

@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.

Awesome. This one looks good. Will wait until the core one gets merged in, and then we can verify the tests on this one and then merge it in as well. Thanks! 🎆

@jpomfret
Copy link
Contributor Author

Awesome! Where do the tests run? Is that the 'Microsoft.PowerShellForGitHub' task that shows as expected?

@HowardWolosky
Copy link
Member

In theory, yes. Those will fail right now however for two reasons:

  1. The core changes aren't in master
  2. There's an unresolved issue with the CI build on DevOps right now which I haven't figured out, so the tests always seem to fail for some reason.

I'll be running them manually for validation before merging, once the Core changes are in.

@jpomfret
Copy link
Contributor Author

Great - thanks for the explanation! I believe I have the core changes have been made and are ready for you.

@HowardWolosky
Copy link
Member

Looks like GitHub's web merge conflict resolver messed with the whitespace in the .psd1 file. Can you take a look and get that fixed up? (Here and in the project cards PR as well)?

@jpomfret
Copy link
Contributor Author

Looks like GitHub's web merge conflict resolver messed with the whitespace in the .psd1 file. Can you take a look and get that fixed up? (Here and in the project cards PR as well)?

I'm not sure what you mean by that, I pulled down the changes and the whitespace all looks ok... e.g.:

image

@HowardWolosky
Copy link
Member

You'll see the problem if you look at the "Files Changed" tab.

image

@jpomfret
Copy link
Contributor Author

I'm sorry - I'm still not following, if I look at the file it all looks lined up.

https://github.com/microsoft/PowerShellForGitHub/blob/68d297e51d9f114d161a44138ede46a125b706af/PowerShellForGitHub.psd1

I do see that it seems to think all the lines have changed, but I don't see why.

@HowardWolosky
Copy link
Member

I do see that it seems to think all the lines have changed, but I don't see why.

Exactly. That's what we want to avoid, because it destroys the ability to diff and clearly see what changed in the file. Some sort of whitespace changed in it, but I can't tell what. It doesn't appear to be the spaces themselves, nor the linefeeds.

The simplest way forward at this point I believe would be to copy the content that's currently in master, commit that (which would remove your changes), and then re-apply the changes you made and commit that again. (Don't do it in one go because git may not notice the whitespace change and may perceive the file as not changed at all).

@jpomfret
Copy link
Contributor Author

Ahh!! Gotcha!
Thanks and sorry, should be fixed here now.

Copy link
Member

@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.

Perfect. Thanks for fixing that psd1. We're going to have the same problem with the last PR as soon as I merge this in, as we'll get merge conflicts in the psd1 again.

@HowardWolosky HowardWolosky merged commit 85170ce into microsoft:master May 12, 2020
@HowardWolosky
Copy link
Member

This is now available publicly in 0.12.0. Thanks so much for your contribution. 🎆

@jpomfret
Copy link
Contributor Author

Yes! This is great, thanks so much for all your coaching!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api completeness This is basic API functionality that hasn't been implemented yet. api-project Work to complete the API's defined here: https://developer.github.com/v3/projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants