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

[chore] - Small cleanup of CircleCi source #1028

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Conversation

ahrav
Copy link
Collaborator

@ahrav ahrav commented Jan 17, 2023

  • Fix some comments.
  • Remove unused args for fxns.
  • Remove deprecated ioutil.ReadAll.

@ahrav ahrav requested review from a team as code owners January 17, 2023 15:35
pkg/sources/github/github.go Outdated Show resolved Hide resolved
pkg/sources/circleci/circleci.go Outdated Show resolved Hide resolved
Comment on lines 112 to 110
func (s *Source) projects(ctx context.Context) ([]project, error) {
func (s *Source) projects() ([]project, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit (optional): I don't see an issue with removing this though it might be more work to add it back if we want to log in these functions (or actually use the context).

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 am going to leave this only because i tend to lean more in the camp of add it when you need it. I fear this could balloon into adding other args that are unused in the hopes they might be used later. I also do acknowledge its context and well in Go you just always pass the context, but it still doesn't sit well with me. There also is this little extra overhead when reading the function to try and map each arg to their usage in the fxn, and when they aren't used it's a little confusing. You tend to re-read the function several times to make sure it's not used.

I suppose a middle ground could be _ context.Context?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I agree, and would be happy with either. Since they're all unexported functions it shouldn't be an issue to add the context back when we need it.

@ahrav ahrav requested review from mcastorina and a team January 17, 2023 17:26
@ahrav ahrav merged commit 319ae64 into main Jan 17, 2023
@ahrav ahrav deleted the chore-circle-ci-small-cleanup branch January 17, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants