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

A type t, equal, show and pp #73

Merged
merged 13 commits into from
Jan 28, 2019
Merged

A type t, equal, show and pp #73

merged 13 commits into from
Jan 28, 2019

Conversation

Leonidas-from-XIV
Copy link
Member

Closes #71.

I think most of it is pretty obvious except for how equality on Assoc would be handled. Since keys can repeat in that association list, I implemented it in a way where same keys must have the same value (thus a custom mem-like function). It is not optimal, since that comparison is O(n^2), but I decided to submit it anyway to have a discussion on whether these semantics are what we want to have to begin with. There are some obvious optimizations but let's take this as starting point.

The reason this is done is because the convention is that the main type
is called `t` and ppx_deriving has special handling when the type is
called `t`.
It looks like ppx_deriving requires at least OCaml 4.04, so might as
well implement show and pp manually.
This should work even with older versions of OCaml
@Leonidas-from-XIV
Copy link
Member Author

The latest commit is green except for a single macOS build which failed for random reasons. Rerunning that particular build it should help but I can't trigger a rebuild. Maybe @pmetzger can?

@NathanReb
Copy link
Collaborator

Thanks for that!

I restarted the failed job and I'll have a look ASAP.

@Leonidas-from-XIV
Copy link
Member Author

Leonidas-from-XIV commented Jan 25, 2019

Looks like it worked this time. Builds on macOS are a bit tiring because they take a long time to start and have a tendency of randomly failing a bit too often.

I haven't included compare because I don't have too much use for it personally, but if there is interest I can also add that in a later PR, by the magic of "stealing whatever ppx_deriving generates".

@Leonidas-from-XIV
Copy link
Member Author

While working on another PR I realized there is sort which might actually be the better solution to determining whether two `Assocs are equivalent, especially since the sort logic exists and due to its existence is the "canonical" way of ordering keys in an `Assoc. Let me know what you think.

Copy link
Collaborator

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

No problem about compare, we can add that later if anyone expresses a need for it, that's already a huge improvement as it is.

I agree sorting the assoc lists based on the key is the way to go here. It will make the code easier to read and will yield a better average time complexity. If performances become an issue even then we can think about improving it but I doubt it'll come to this.
There will be undefined behavior with objects having multiple occurrences of the same key but the current implementation will also yield some odd results in those cases and I don't think we should bother treating those.

What would you think about adding unit tests for those? Especially for equal since it has some custom logic that one might inadvertently break.
I think that might be the right time to add some. Your help on this is already much appreciated and if you do not wish to bother writing tests I'll just take care of it later.

@Leonidas-from-XIV
Copy link
Member Author

I will add unit tests to it (and actually it is way nicer now, since I have an equality and a pretty printer that I can use in the tests), but since that takes a bit of time I submitted the other changes that you requested since they were quick to do.

@Leonidas-from-XIV
Copy link
Member Author

After adding the deprecation annotation I had to change all occurrences of json to t to not trigger deprecation warnings ourselves.

Copy link
Collaborator

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

I added a few suggestions, let me know what you think!

This allows duplicate keys to compare equal as long as they are in the
same order respectively.
It is also more consistent with Assoc now.
This PR has the advantage that it is now easy to create a "testable"
since that requires an "equal" and "pp" and this PR is exactly about
adding those.
@Leonidas-from-XIV
Copy link
Member Author

I added some unit tests, though I didn't bother to test the non-standard extensions to JSON. It uses alcotest which I added as a test-only dependency.

I think the CI scripts will run the tests automatically, though we might possibly disable them on all but one build because running the tests takes a while because it rebuilds it with test dependencies.

@NathanReb
Copy link
Collaborator

Thanks for your time and effort on this, it's much appreciated!

About the CI, I'm not sure this will have significant impact on the already insanely long CI runs. I think dropping CI builds for versions older than 4.04, even maybe 4.05 would make sense.

The osx builds are also way too long, I'll look into this but I might only test osx with the latest ocaml version for instance.

Anyway, I digress, tyvm, merging!

@NathanReb NathanReb merged commit 6222f94 into ocaml-community:master Jan 28, 2019
@Leonidas-from-XIV Leonidas-from-XIV deleted the type-t-equal-show branch January 28, 2019 16:32
NathanReb pushed a commit to NathanReb/opam-repository that referenced this pull request Jan 30, 2019
CHANGES:

*2019-01-30*

### Deprecate

- `json` types are deprecated in favor of their new `t` aliases, ahead of their removal in the next
  major release (ocaml-community/yojson#73, @Leonidas-from-XIV)

### Add

- Add a type `t` and monomorphic `equal`, `pp` and `show` (ocaml-community/yojson#73, @Leonidas-from-XIV)
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request Feb 6, 2019
Change at Deducteam#133 didn't fix the issue Deducteam#131 properly, indeed
ocaml-community/yojson#73 forces us to use
1.6.0.

Closes Deducteam#131
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request Feb 6, 2019
Change at Deducteam#133 didn't fix the issue Deducteam#131 properly, indeed
ocaml-community/yojson#73 forces us to use
1.6.0.

Closes Deducteam#131
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.

2 participants