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 literal true and false types #117

Merged
merged 6 commits into from May 9, 2022
Merged

Support literal true and false types #117

merged 6 commits into from May 9, 2022

Conversation

danog
Copy link
Contributor

@danog danog commented May 4, 2022

This PR adds support for literal true and false types.

@romm
Copy link
Member

romm commented May 4, 2022

Hi @danog thank you very much!

I wanted to add some things but I can't write to the repository because it is owned by an organization, see https://github.community/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847 ; could you add me as a maintainer to this very repository, as it was done there?

Thanks!

@danog
Copy link
Contributor Author

danog commented May 4, 2022

Done!
Though I also wouldn't have minded if you closed this PR and did a manual merge on a feature branch of this repo :)

@romm
Copy link
Member

romm commented May 4, 2022

Perfect, thanks!

Just pushed, I'll let you some time to review the changes and tell me if everything is ok by your side. 🙂

@danog
Copy link
Contributor Author

danog commented May 4, 2022

Looks great, thanks!
Can I haz a tag after the merge? :3

@romm
Copy link
Member

romm commented May 4, 2022

Actually I missed one thing: TrueType and FalseType should both be implementing ScalarType; missing methods should be implemented and tested as well. Do you want to try to do it by yourself? Otherwise I'll take time to do it, no worries.

Anyway when it's ready I'll gladly publish a new release, that is well deserved!

@danog
Copy link
Contributor Author

danog commented May 4, 2022

I'll do it tomorrow, no worries!

@danog
Copy link
Contributor Author

danog commented May 5, 2022

Now that I took a look at it, maybe extending ScalarType isn't such a good idea after all: like with the literal null type, which doesn't extend ScalarType, we want to treat only the literal null case, without handling or supporting casts, which would make little sense given that we explicitly specified a literal type.

@romm
Copy link
Member

romm commented May 5, 2022

I understand your concern; null is not a scalar because… well it's not a scalar type that's all. Concerning true and false, in order to be consistent with the BooleanType we need to allow them to accept values like 1 or 0. I made this choice at the beginning of the project, it was already challenged in #3 and will be part of a major change in a next release.

In the meantime, we need to be consistent with the existing features. 🙂

@romm
Copy link
Member

romm commented May 9, 2022

Hi @danog I changed to boolean types to be handled as scalar; I also changed some things to be more consistent with other "value types" (FloatValueType / IntegerValueType / StringValueType).

Anyway, thanks for the original work!

@romm romm merged commit afcedf9 into CuyZ:master May 9, 2022
@romm
Copy link
Member

romm commented May 9, 2022

There you go sir 🙂

https://github.com/CuyZ/Valinor/releases/tag/0.8.0

@danog
Copy link
Contributor Author

danog commented May 9, 2022

Yay, thanks, almost forgot about this PR :)

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.

None yet

2 participants