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

overwrite status. #20

Merged
merged 5 commits into from
Oct 3, 2023
Merged

overwrite status. #20

merged 5 commits into from
Oct 3, 2023

Conversation

yseto
Copy link
Contributor

@yseto yseto commented Sep 13, 2023

No description provided.

@yseto yseto self-assigned this Sep 13, 2023
@coveralls
Copy link

coveralls commented Sep 13, 2023

Coverage Status

coverage: 82.278% (-17.7%) from 100.0% when pulling cea26f1 on support-as-status into 490c13a on master.

@yseto yseto marked this pull request as ready for review September 25, 2023 07:49
asstatusparse.go Outdated Show resolved Hide resolved
asstatusparse.go Show resolved Hide resolved
asstatusparse.go Outdated Show resolved Hide resolved
asstatusparse.go Outdated Show resolved Hide resolved
Co-authored-by: KADOTA, Kyohei <lufia@users.noreply.github.com>
},
{
name: "argument error",
source: "invalid=critical=invalid",
Copy link
Contributor

Choose a reason for hiding this comment

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

This case emits an error by invalid, not the second =.

@yseto yseto merged commit 1fd6e2d into master Oct 3, 2023
10 checks passed
@yseto yseto deleted the support-as-status branch October 3, 2023 01:14
@itchyny
Copy link
Contributor

itchyny commented Oct 4, 2023

Although the API changes have been released, I'd like to share my slight concerns. First of all, about naming of Parse. Some packages (time, net/url, x/net/html, etc) expose APIs with the same name, but their results are very trivial; they parses strings to return the main structs of the packages. But no one can guess what checkers.Parse returns correctly. It could be *checkers.Checker or checkers.Status but how one can tell it is a map of checkers.Status? So I would propose to rename it to ParseStatusMap. Secondly, this is more smaller issue, but the file name asstatusparse.go explains how the status maps will be used, not what in the file does. I think you could name better, something like statusmap.go.

@lufia
Copy link
Member

lufia commented Oct 5, 2023

Sure...we published v0.1.0 that includes Parse function.

Therefore we will need to do:

  1. add func ParseStatusMap
  2. mark Parse as // Deprecated:
  3. add retract v0.1.0 to go.mod
  4. publish new version

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