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

support id, path or object of groups and project #68

Merged
merged 20 commits into from
Oct 22, 2019
Merged

support id, path or object of groups and project #68

merged 20 commits into from
Oct 22, 2019

Conversation

jetersen
Copy link
Collaborator

Now the user of the library does not have to worry about path encoding of group and project.

I also tried to unify query parameters and requests objects and methods.

fixes #18
fixes #33
fixes #66

@codecov-io
Copy link

codecov-io commented Sep 22, 2019

Codecov Report

Merging #68 into master will increase coverage by 8.81%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage    24.2%   33.01%   +8.81%     
==========================================
  Files         127      122       -5     
  Lines        1636     1575      -61     
  Branches      120        0     -120     
==========================================
+ Hits          396      520     +124     
+ Misses       1160     1055     -105     
+ Partials       80        0      -80
Impacted Files Coverage Δ
...iClient/Models/Users/Requests/UpdateUserRequest.cs 0% <ø> (ø) ⬆️
...Projects/Requests/UpdateProjectMilestoneRequest.cs 0% <ø> (ø) ⬆️
...lient/Models/Issues/Requests/UpdateIssueRequest.cs 0% <ø> (ø) ⬆️
...odels/MergeRequests/Requests/AcceptMergeRequest.cs 0% <ø> (ø) ⬆️
...lient/Models/Groups/Requests/UpdateGroupRequest.cs 0% <ø> (ø) ⬆️
...els/Groups/Requests/UpdateGroupMilestoneRequest.cs 0% <ø> (ø) ⬆️
...els/Projects/Requests/UpdateProjectLabelRequest.cs 0% <ø> (ø) ⬆️
...tLabApiClient/Models/Releases/Responses/Release.cs 0% <ø> (ø) ⬆️
...odels/MergeRequests/Requests/UpdateMergeRequest.cs 0% <ø> (ø) ⬆️
...t/Models/Branches/Requests/ProtectBranchRequest.cs 0% <ø> (ø) ⬆️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fde4e25...a46aa64. Read the comment docs.

@jetersen
Copy link
Collaborator Author

@nmklotas Hi Julius.

I know this is a bigger PR, do you have any concerns with the changes. I know I am breaking the API a lot lately, I might look into cleaning up Milestone and Note API to be a separate API client with a interface since they are shared between Groups and Projects. See https://github.com/jdalrymple/node-gitlab/blob/master/src/core/templates/ResourceMilestones.ts.

Could also adopt more of the template resources like members.

I tried to align the methods.

@jetersen
Copy link
Collaborator Author

jetersen commented Oct 3, 2019

Hi @nmklotas I would appreciate a review or a merge 😅

Just ran into not being able to call Projects.GetAsync with string

        public async Task<Project> GetProject(string projectId)
        {
            return await _client.Projects.GetAsync(projectId);
        }

@nmklotas
Copy link
Owner

Thanks for this big beutiful PR :) sorry for the delay.

I think we need to change few things.

  1. Don't use projectBaseUrl() or other BaseUrl() methods, we should embrace the declarative nature of strings then it's very easy to mind map them to gitlab and its docs.

  2. For parameters we should use a simple class, let's call it ProjectId, untyped object might cause problems.

it should have these properties:

ToString() overriden to create string for int, or encode if it's the path
implicit operator to create it from string
implicit operator to create it from int

so user can simply pass int or string and these operators would created class ProjectId object.
Then it's typed and easy to use and easy to maintain.

and since we are overriding ToString() it can be used in the path & it converts automatically.

what do you think?

@jetersen
Copy link
Collaborator Author

Sounds totally reasonable! 👍

@nmklotas
Copy link
Owner

You should have coloborator access for now :) , so you can work without me impeding you

@jetersen
Copy link
Collaborator Author

Thanks 👍

@jetersen jetersen mentioned this pull request Oct 11, 2019
2 tasks
Copy link
Collaborator Author

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

Whoops 😅

@jetersen
Copy link
Collaborator Author

Travis CI come on gotta support dotnet core 3...

@jetersen
Copy link
Collaborator Author

There we go 😆 New and improved code coverage 👍

@jetersen
Copy link
Collaborator Author

jetersen commented Oct 14, 2019

Docker works fine locally, but Travis and appveyor both have blocking issues.

I think with appveyor using gitlab 11 which still uses postgres 9 we got closer :)

@jetersen
Copy link
Collaborator Author

Okay cleaned up the branch to only handle the API cleanup.

Moving the test setup fixes into a separate PR.

@jetersen jetersen merged commit baa9da1 into nmklotas:master Oct 22, 2019
@jetersen jetersen deleted the cleanAPI branch October 22, 2019 05:17
@jetersen
Copy link
Collaborator Author

@nmklotas Hope you don't mind Julius that I used my new found powers to release a new version 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants