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

add files and trees client #77

Merged

Conversation

rrampen
Copy link
Contributor

@rrampen rrampen commented Oct 25, 2019

Came across this very helpful tool. Missed the possibility to read the content from files. Added this in this PR.

@rrampen rrampen force-pushed the rrampen/feature_add_loading_file_content branch from 5ac7dd8 to dd735a0 Compare October 25, 2019 12:44
@jetersen
Copy link
Collaborator

link to see current commit author: https://github.com/nmklotas/GitLabApiClient/pull/77/commits/dd735a058f6546f32b2e93e588ebce8a449b393c.patch

@rrampen
You might want to set up an email alias on your GitHub account 🥇
https://help.github.com/articles/adding-an-email-address-to-your-github-account/

Or look into fixing your git user.email config 😅
https://help.github.com/articles/setting-your-commit-email-address-in-git
then you would need to amend your commits: https://stackoverflow.com/a/3042512
after changing author

to get proper credit for your commits 🏆

Copy link
Collaborator

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Overall looks really good, would be good if you could add XML comments to each public method.

src/GitLabApiClient/FileClient.cs Outdated Show resolved Hide resolved
src/GitLabApiClient/FileClient.cs Outdated Show resolved Hide resolved
src/GitLabApiClient/TreesClient.cs Outdated Show resolved Hide resolved
src/GitLabApiClient/FileClient.cs Outdated Show resolved Hide resolved
src/GitLabApiClient/TreesClient.cs Outdated Show resolved Hide resolved
src/GitLabApiClient/TreesClient.cs Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 8, 2019

Codecov Report

Merging #77 into master will increase coverage by 0.75%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   71.86%   72.61%   +0.75%     
==========================================
  Files         133      139       +6     
  Lines        1930     1983      +53     
==========================================
+ Hits         1387     1440      +53     
  Misses        543      543
Impacted Files Coverage Δ
src/GitLabApiClient/TreesClient.cs 100% <100%> (ø)
...piClient/Models/Trees/Requests/TreeQueryOptions.cs 100% <100%> (ø)
src/GitLabApiClient/Models/Trees/Responses/Tree.cs 100% <100%> (ø)
src/GitLabApiClient/FilesClient.cs 100% <100%> (ø)
src/GitLabApiClient/Models/Files/Responses/File.cs 100% <100%> (ø)
...tLabApiClient/Internal/Queries/TreeQueryBuilder.cs 100% <100%> (ø)
src/GitLabApiClient/GitLabClient.cs 100% <100%> (ø) ⬆️
... and 3 more

_httpFacade = httpFacade;
_fileQueryBuilder = fileQueryBuilder;
}
public async Task<File> GetAsync(ProjectId projectId, string filePath, Action<FileQueryOptions> options = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just tested the file api, as ref option is mandatory I believe it is best to have it as a parameter and not as an option, that is somewhat hidden.

The Files API only have a ref query option, doc even says it is required
https://docs.gitlab.com/ee/api/repository_files.html#get-file-from-repository

sadly enough this is not true for the tree api: https://docs.gitlab.com/ee/api/repositories.html it will assume default branch if ref is not provided. Gotta love GitLab's inconsistent API 😆

So I suggest for now we remove the FileQueryOptions and just switch to projects/{projectId}/repository/files/{filePath.UrlEncode()}?ref={reference}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice to provide an option to convert File to Content by handling the decoding of base64.

public string Path { get; set; }
public bool Recursive { get; set; }

internal TreeQueryOptions(string reference = null, string path = null, bool recursive = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recursive should not default to true

Suggested change
internal TreeQueryOptions(string reference = null, string path = null, bool recursive = true)
internal TreeQueryOptions(string reference = null, string path = null, bool recursive = false)

@jetersen jetersen changed the title [dev] added feature to load files from repo add files and trees client Jan 1, 2020
@jetersen jetersen merged commit c48ea9a into nmklotas:master Jan 1, 2020
@jetersen
Copy link
Collaborator

jetersen commented Jan 2, 2020

@rrampen thanks for your first open source contribution 👏

Hope you don't mind that I finished the feature 🧙‍♂️

@rrampen
Copy link
Contributor Author

rrampen commented Jan 7, 2020

@rrampen thanks for your first open source contribution

Hope you don't mind that I finished the feature 🧙‍♂️

@Casz On the contrary I am grateful that you took the effort to get this into master :-)

MindaugasLaganeckas pushed a commit to MindaugasLaganeckas/GitLabApiClient that referenced this pull request Mar 9, 2021
Co-authored-by: Joseph Petersen <josephp90@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants