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

Use constructors for static tokens #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Apr 12, 2024

WARNING: This commit includes breaking changes.

Instead of global variables, use function constructors for static tokens such as Null, ObjectStart, ObjectEnd, ArrayStart, and ArrayEnd. This provides a greater degree of immutability for the API.

Note that static tokens for True and False are dropped in favor of the pre-existing Bool constructor.
If necessary, we can always re-introduce True and False.

WARNING: This commit includes breaking changes.

Instead of global variables, use function constructors
for static tokens such as Null, ObjectStart, ObjectEnd,
ArrayStart, and ArrayEnd. This provides a greater degree
of immutability for the API.

Note that static tokens for True and False are dropped
in favor of the pre-existing Bool constructor.
If necessary, we can already re-introduce True and False.
@dsnet dsnet requested review from mvdan and johanbrandhorst April 12, 2024 04:58
Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Personally I might have left it as it was before - lots of std packages have the immuntability issue with e.g. var ErrFoo = ..., and in practice it's not an issue. But SGTM either way.

@dsnet
Copy link
Collaborator Author

dsnet commented Apr 12, 2024

One advantage of this is that it makes Null at the same level of visibility as Bool, Int, etc.

In godoc, variables are given less prominence than functions (e.g., variables never show up in the table of contents, while functions do).

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

I think I like this better even if historically I take the point Dan makes

@ChrisHines
Copy link

I think it is fine. We did the same thing for "constant" values in go-kit/log/level.

https://github.com/go-kit/log/blob/0b69c7049332e99c25d5fd0f4d08317cfe45e7d8/level/level.go#L204-L239

@mvdan
Copy link
Collaborator

mvdan commented Apr 18, 2024

SGTM with the assumption the compiler is/will be clever enough to handle this efficiently

@Zxilly
Copy link

Zxilly commented Jul 3, 2024

I prefer the old scheme. If it's for immutability, can we set these enums to const string and expose the rawToken helper function?

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.

5 participants