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 sleet #3306

Merged
merged 1 commit into from
Dec 4, 2020
Merged

Add sleet #3306

merged 1 commit into from
Dec 4, 2020

Conversation

daisy1754
Copy link
Contributor

Make sure that you've checked the boxes below before you submit PR:

  • I have added my package in alphabetical order.
  • I have an appropriate description with correct grammar.
  • I know that this package was not listed before.
  • I have added pkg.go.dev link to the repo and to my pull request.
  • I have added coverage service link to the repo and to my pull request.
  • I have added goreportcard link to the repo and to my pull request.
  • I have read Contribution guidelines, maintainers note and Quality standard.

@avelino
Copy link
Owner

avelino commented Oct 19, 2020

Thank you for contributing with awesome-go, we will revise your contribution as soon as possible.

Automation body links content check:

  • godoc.org or pkg.go.dev: True
  • goreportcard.com: True
  • coverage: True

your project is about to be approved, it's under revision, it may take a few days

Copy link
Owner

@avelino avelino left a comment

Choose a reason for hiding this comment

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

review:

  • coverage is 70.95%

[...] then coverage should be >= 80% for non-data-related packages and >=90% for data related packages [...]

read more: https://github.com/avelino/awesome-go/blob/master/CONTRIBUTING.md#quality-standards

@daisy1754
Copy link
Contributor Author

working on adding test. For golint, it seems guideline isn't super explicit about requirement. Is 100% golint requirement? I want to avoid having redundunt comment like below

// TransactionType transaction type
TransactionType

@daisy1754
Copy link
Contributor Author

FWIW author of lint suggests ignoring lint when it's not relevant golang/lint#186 (comment)

@avelino
Copy link
Owner

avelino commented Nov 8, 2020

We don't ask for 100% lint, we recommend that you have all lint items corrected to keep the quality of your package to a minimum.

In my view if it is possible to correct, why not correct?

@daisy1754
Copy link
Contributor Author

IMO in some cases (please refer to my example above), "fixing" lint would actually do more harm than benefit. When variable name is self explanatory I prefer not to have comment.

Anyways we're working on increasing coverage. I'll update here once it's ready

@daisy1754
Copy link
Contributor Author

@avelino coverage is now 80%+ https://codecov.io/gh/BoltApp/sleet
I went thorough and fixed lint mostly, but didn't add some redundunt comments so score is not 100 (but I believe this is better than adding redundunt comment just for sake of lint score).

Please kindly review again.

@avelino avelino merged commit 2130e9b into avelino:master Dec 4, 2020
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.

2 participants