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

Various cleanup fixes #20

Merged
merged 19 commits into from
Aug 13, 2020
Merged

Various cleanup fixes #20

merged 19 commits into from
Aug 13, 2020

Conversation

luxas
Copy link
Collaborator

@luxas luxas commented Aug 12, 2020

This PR:

  • Removes the now unnecessary doc.go file
  • Fixes desired state reconciliation, by selecting what fields are considered "spec" when comparing two objects
  • Fixes some error handling, nil checks, typos, and linter errors
  • Adds many new linters run by golangci-lint
  • Removes unnecessary internal casting, to avoid relying on runtime-cast lookups for things we know the type of.
  • Adds more unit tests
  • Splits CreatableInfo into the more descriptive InfoRequest and DefaultedInfoRequest, which *Info structs should implement
  • Adds more badges to the README 🚀

@luxas luxas added this to the v0.0.1 milestone Aug 12, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2020

Codecov Report

Merging #20 into master will increase coverage by 0.71%.
The diff coverage is 41.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   52.86%   53.57%   +0.71%     
==========================================
  Files          21       22       +1     
  Lines         978     1034      +56     
==========================================
+ Hits          517      554      +37     
- Misses        429      450      +21     
+ Partials       32       30       -2     
Impacted Files Coverage Δ
github/auth.go 40.78% <ø> (ø)
github/client.go 68.00% <ø> (ø)
github/client_organization_teams.go 0.00% <0.00%> (ø)
github/client_organizations.go 71.42% <0.00%> (ø)
github/client_repositories_user.go 0.00% <0.00%> (ø)
github/client_repository_deploykey.go 0.00% <0.00%> (ø)
github/client_repository_teamaccess.go 0.00% <0.00%> (ø)
github/resource_deploykey.go 0.00% <0.00%> (ø)
github/resource_teamaccess.go 18.18% <0.00%> (-1.82%) ⬇️
gitprovider/enums.go 100.00% <ø> (ø)
... and 12 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 4ed5520...695828d. Read the comment docs.

@luxas luxas marked this pull request as ready for review August 13, 2020 13:01
@luxas
Copy link
Collaborator Author

luxas commented Aug 13, 2020

@twelho @stefanprodan ready for review & merge 😄

Copy link

@twelho twelho left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise LGTM 👍

@@ -223,6 +224,7 @@ func NewClient(ctx context.Context, optFns ...ClientOption) (gitprovider.Client,
// the default.
var gh *github.Client
var domain string

Copy link

Choose a reason for hiding this comment

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

Unnecessary space?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I started adding all these spaces because of the whitespace linter wsl, but then was just like "nah, I don't care" and left half-way through 😂
I think I'll live things as-is now, and someone can later make whitespace consistent across the project.
Low-hanging fruit anyways.

bitbucket/doc.go Outdated
@@ -17,5 +17,6 @@ limitations under the License.
package bitbucket

import (
// Dummy import until we have the implementation ready.
Copy link

Choose a reason for hiding this comment

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

Maybe add a TODO.

.golangci.yml Outdated
# - godox don't error although there are TODOs in the code
- funlen
- whitespace
# - wsl allow "non-ideomatic" whitespace formattings for now
Copy link

Choose a reason for hiding this comment

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

ideomaticidiomatic

@@ -99,6 +103,7 @@ func (c *TeamsClient) List(ctx context.Context) ([]gitprovider.Team, error) {
if err != nil {
return nil, err
}

Copy link

Choose a reason for hiding this comment

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

Another space character here.

@@ -73,6 +74,7 @@ func (c *OrganizationsClient) List(ctx context.Context) ([]gitprovider.Organizat
if apiObj.Login == nil {
return nil, fmt.Errorf("didn't expect login to be nil for org: %+v: %w", apiObj, gitprovider.ErrInvalidServerData)
}

Copy link

Choose a reason for hiding this comment

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

Space

return false, nil
}
// Otherwise, make the desired state the actual state
return true, r.Update(ctx)
}

// Delete deletes the current resource irreversebly.
// Delete deletes the current resource irreversible.
Copy link

Choose a reason for hiding this comment

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

irreversibly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh I just changed this based on the linter's suggestion without even looking at it 😂

gitlab/doc.go Outdated
@@ -17,5 +17,6 @@ limitations under the License.
package gitlab

import (
// Dummy import until we have the implementation ready.
Copy link

Choose a reason for hiding this comment

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

Maybe add TODO.

type GenericDeletable interface {
// Delete deletes the current resource irreversebly.
type Deletable interface {
// Delete deletes the current resource irreversible.
Copy link

Choose a reason for hiding this comment

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

irreversibly

@luxas
Copy link
Collaborator Author

luxas commented Aug 13, 2020

Addressed comments, ready to merge.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Nice cleanup 🏅

@luxas luxas merged commit bf2a7a6 into master Aug 13, 2020
@luxas luxas deleted the cleanup branch August 13, 2020 15:37
@luxas luxas mentioned this pull request Aug 14, 2020
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.

4 participants