-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add core functionality for Project API's #160
Conversation
Issue was because I didn't give my token the right access :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow....this is GREAT! Thanks so much for putting in all this great work. Very much appreciated! I'm sure that this will help out a bunch of users.
I think that you did a great job adhering to the overall project style. I've left a number of comments for you to review and address.
Again...thanks so much for all of this!
Tests/GitHubProjects.tests.ps1
Outdated
$results = Get-GitHubProject -Project $repoProject.id | ||
} | ||
catch { | ||
$null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the scenario you're working around here? At the very least, this deserves a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Get
was failing to find the project and when using the -Project
parameter it threw an error which messed up the tests.
Changed these tests around to test for the thrown error instead... new version incoming :)
Thank you so much for the code review! Working through the changes. |
force pushed because I made the same changes in my local repo as I committed from the review (didn't realise what I was doing was actually making the changes, sorry if that broke anything!) |
Hey @HowardWolosky - I have another piece of work ready to PR for Project Columns, wondering if I should lump it into this PR or if you'd rather keep it seperate? Also - let me know if I'm still outstanding on any review items for this core project piece - I believe everything has been addressed. Thanks! |
@jpomfret, please keep them separate. I'd also recommend, if possible, to do it in a new branch off of master as opposed to a new branch off of I'll be continuing my review of this PR tomorrow. Thanks again for the contributions....Looking forward to seeing the additional work! |
Thanks @HowardWolosky - I submitted two new PR's for the other pieces. I'll be around tomorrow for any fixes you need. (I tried really hard on spacing and brackets.. but I'm sure you'll find some 😄) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
Just a few minor additional corrections to make, then we can merge this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final updates look good.
Locally ran all tests and they passed.
This looks good to merge.
🎆 🚢 🎆
This is now available publicly in 0.12.0. Thanks so much for your contribution. 🎆 |
Adding support for projects core functionality
Also added test coverage - all tests passed on my machine, I did have issues with the last call to remove the repository.
Resolves #159