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

Support custom markers #76

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Support custom markers #76

merged 5 commits into from
Apr 8, 2024

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Mar 30, 2024

fix: #70

@zirain
Copy link
Contributor Author

zirain commented Apr 5, 2024

@thbkrkr PTAL

@thbkrkr
Copy link
Contributor

thbkrkr commented Apr 5, 2024

Hi @zirain, I saw your PR but it's been a super busy week. It looks good in principle.

We haven't been very good at updating the doc recently with all possible features. What do you think about adding a features section at the bottom of the readme and briefly detailing this new one?

@thbkrkr thbkrkr added the enhancement New feature or request label Apr 5, 2024
@zirain
Copy link
Contributor Author

zirain commented Apr 5, 2024

Hi @zirain, I saw your PR but it's been a super busy week. It looks good in principle.

We haven't been very good at updating the doc recently with all possible features. What do you think about adding a features section at the bottom of the readme and briefly detailing this new one?

sure, will do it later.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@thbkrkr thbkrkr changed the title support custom markers Support custom markers Apr 8, 2024
continue
}

if err := registry.Define(marker.Name, t, struct{}{}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support custom markers with value?

I think that by defining the custom marker with the output type struct{}{}, it is not possible to have a custom marker with a value. It is kind of a shame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's start with no-value marker, we can do that later?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can do that later.

@thbkrkr thbkrkr merged commit 284491a into elastic:master Apr 8, 2024
2 checks passed
@zirain zirain deleted the custom-markers branch April 8, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] support more custom markers
2 participants