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 Get Repository.Contents functionality (take 2) #146

Merged
merged 18 commits into from
Apr 3, 2020

Conversation

Shazwazza
Copy link
Contributor

Original PR is here #143

But during testing my fork got deleted so this is a re-push of those files, plus tests and everything fixed up.

I still need to write a few more tests but please let me know if there's anything that is obvious that i need to fix up.

Learning a lot about Powershell here :) got the static analysis running and tests 🎉

@Shazwazza
Copy link
Contributor Author

@HowardWolosky I've discovered something interesting regarding your previous question of

Get-GitHubContents -OwnerName Microsoft -RepositoryName PowerShellForGitHub -Path README.md -MediaType Raw

vs

Invoke-GHRestMethodMultipleResult -Description 'Getting Contents' -UriFragment /repos/Microsoft/PowerShellForGitHub/contents/README.md -AcceptHeader 'application/vnd.github.v3.Raw

As it turns out there is actually 3 different results that GH will return. The reason you are seeng a json structure returned with application/vnd.github.v3.Raw is because the casing of this header is invalid and it must be application/vnd.github.v3.raw. When raw is used, it returns the raw bytes of the file. When html is used it returns the converted md -> html bytes of the file. When anything else is used that is not an exact match, then the json document is returned which contains the base64 encoded version of the file.

I'll update to support all 3.

@HowardWolosky
Copy link
Member

Thanks for the update. Will review this PR later this week.

@Shazwazza Shazwazza marked this pull request as ready for review February 5, 2020 00:54
@Shazwazza
Copy link
Contributor Author

Sounds good, i've just pushed the latest update along with unit tests to cover the scenarios. There's a new switch -ResultAsString which will auto decode the resulting bytes or base64 string to a resulting string. There's tests to show this 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.

Thanks for the update, @Shazwazza. I loved seeing those really robust tests. Thanks for that! There is some additional feedback here for you to address. Most of it is quite minor/formatting-related. The one that's really bothering me though is the confusing nature of having both Get-MediaAcceptHeader and Get-ContentMediaType. I provided some additional thoughts on that, and would be interested in hearing your thoughts as well.

Many thanks into digging into the reason behind the unexpected encoded results that I was finding earlier when I was validating #143.

GitHubContents.ps1 Outdated Show resolved Hide resolved
GitHubContents.ps1 Outdated Show resolved Hide resolved
GitHubCore.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Outdated Show resolved Hide resolved
GitHubCore.ps1 Outdated Show resolved Hide resolved
GitHubContents.ps1 Outdated Show resolved Hide resolved
GitHubContents.ps1 Show resolved Hide resolved
@Shazwazza
Copy link
Contributor Author

Hi @HowardWolosky thanks for the review, will fix up now. I think the auto-formatting in VS code is causing some of the formatting issues. I'll see what can be done about that and reply to your other questions inline.

@Shazwazza
Copy link
Contributor Author

Hi @HowardWolosky I've pushed a new round of updates:

  • Streamlines Get-MediaAcceptHeader to support all scenarios and updates usages accordingly
  • Adds .editorconfig and .vscode/settings.json to deal with auto formatting in vscode
  • Adds Uri parameter and adds tests for it

@HowardWolosky
Copy link
Member

@Shazwazza - I completely missed that there was an update to this PR. I thought I was still waiting on an update from you to review. My apologies. I will check out your updates this week. So sorry about that.

@Shazwazza
Copy link
Contributor Author

Hi @HowardWolosky , any updates on this one? Thanks!

@HowardWolosky
Copy link
Member

@Shazwazza - Quite sorry about that. I promise to finish up the review this week. Things got really hectic and harder to schedule time for recently with everything that's going on. Hope you're staying safe, and I look forward to getting this change in shortly. Thanks again for your patience.

@Shazwazza
Copy link
Contributor Author

No worries and hope you are well too. Hopefully I'll be able to get some more functionality added to this project in the coming months. 🤞

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.

Updates all looked good.
Super minor feedback and two failing UT's.
I promise a significantly quicker turnaround with your next PR update.

.editorconfig Show resolved Hide resolved
.vscode/settings.json Show resolved Hide resolved
GitHubCore.ps1 Outdated Show resolved Hide resolved
GitHubCore.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Show resolved Hide resolved
Tests/GitHubContents.tests.ps1 Show resolved Hide resolved
Shazwazza and others added 4 commits April 3, 2020 16:09
@Shazwazza
Copy link
Contributor Author

@HowardWolosky Hi, i've pushed some changes. All tests are passing now. The problem is the hard coded file content was out of date. If we can get this part merged then I can work on create/update file apis so we don't have to worry about that. Let me know if all is good :)

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.

Let's get this merged in! 🎆
I'll get a new release pushed out to PowerShellGallery later today.

Thanks so much for this contribution. Looking forward to seeing more from you!

@HowardWolosky HowardWolosky merged commit 9a45908 into microsoft:master Apr 3, 2020
@HowardWolosky
Copy link
Member

This has now been published out as part of 0.11.0. Thanks again for your contribution.

@HowardWolosky
Copy link
Member

@Shazwazza - You can see in #183 that these tests are failing again because the content has changed. Are you still planning on working on some of the other Content API's so that we'd be able to set the content to make this less fragile? You might be able to get some ideas from my Gist Draft PR #172.

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.

2 participants