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

Include a log when creating a new labels about what those labels are #121

Merged
merged 3 commits into from
Jan 3, 2019

Conversation

orta
Copy link
Contributor

@orta orta commented Jan 2, 2019

What Changed

$ yarn auto create-labels
$

->

$ yarn auto create-labels
Created labels: [x, y, z]
$

Why

I didn't know what the labels would be, and even after creating them I didn't.

I couldn't find any other uses of the logger.log which makes me think this is the wrong way to do it?

Todo:

  • Add tests
  • Add docs (n/a)
  • Add SemVer label

@orta
Copy link
Contributor Author

orta commented Jan 2, 2019

Alternative take: also print out a url like: https://github.com/omakase-js/omakase/labels so that people can inspect?

@hipstersmoothie hipstersmoothie added the patch Increment the patch version when merged label Jan 2, 2019
@hipstersmoothie
Copy link
Collaborator

Awesome! I like the idea of printing the URL also. I'll merge once you add that 👍

logger.log is fine, I think we just hadn't found an area to use it where it made sense. Thanks for all the contributions!

@codecov
Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #121 into master will not change coverage.
The diff coverage is 84.61%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   75.02%   75.02%           
=======================================
  Files          14       14           
  Lines         973      973           
  Branches      137      137           
=======================================
  Hits          730      730           
  Misses        214      214           
  Partials       29       29
Impacted Files Coverage Δ
src/github-release.ts 96.39% <100%> (ø) ⬆️
src/git.ts 98.18% <50%> (ø) ⬆️

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 a7803b6...f171ebf. Read the comment docs.

@orta orta force-pushed the include_log_for_creating_labels branch from def3e0e to 6dcb3d3 Compare January 2, 2019 22:09
Copy link
Collaborator

@hipstersmoothie hipstersmoothie left a comment

Choose a reason for hiding this comment

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

Looks great! Just one typedef is a little off

src/git.ts Outdated

return Promise.resolve();
}

// Looks like: https://developer.github.com/v3/repos/#get
public async getRepoMetadata(): Promise<any> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks like this.ghub.repos.get return a type of GHub.Response<GHub.ReposGetResponse> so the return type for this function should probably be Promise<GHub.Response<GHub.ReposGetResponse>>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public async getRepoMetadata(): Promise<any> {
public async getRepoMetadata(): Promise<GHub.Response<GHub.ReposGetResponse>> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, if I remove the return type annotation it'll be inferred correctly - I've removed that 👍

@orta orta force-pushed the include_log_for_creating_labels branch from 6dcb3d3 to f171ebf Compare January 3, 2019 14:29
@hipstersmoothie hipstersmoothie merged commit 5833aa8 into intuit:master Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants