search

NathanReb / ppx_yojson

OCaml PPX extension for JSON literals and patterns
BSD 2-Clause "Simplified" License
42 stars 5 forks source link

Removed the Raise commands and embedded error_extensionf for unsupported payload error #35

Closed Anu-123-gif closed 1 year ago

Anu-123-gif commented 2 years ago

Hey @ayc9 & @pitag-ha I have removed the Raise commands and embedded error_extensionf for all errors (except 3-4 which are creating issues) in this PR.

pitag-ha commented 2 years ago

Hey @Anu-123-gif, thanks for your work, it looks very good! Btw, don't worry about the CI! It doesnt' seem like the reason why the CI is failing is related to your PR. There seem to go wrong two things:

And just one detail about the code: as already said, it looks very good! One thing that might be nice improving would be to factor out the creation of the error extension node into a separate function and use that function at the places where the error occurs. What do you think?

pitag-ha commented 2 years ago

Oh, I've just remembered that I didn't know what "CI" was when I started my Outreachy internship. The scope of "CI" depends a bit on each project; here, I was just referring to the testing mechanism you can see below: you can see below that "PR number update" has passed, whereas "ocaml-ci" has failed. ocaml-ci tests the changes made in PRs in an automated way, e.g. it checks if the code is formatted and if the tests keep on passing.

ayc9 commented 2 years ago

I agree it looks good! I would add that it might be worth it to also remove the Raise functions now that they are not used, and to also include the updated tests to the pr.

Anu-123-gif commented 2 years ago

btw, have you formatted your code (i.e. have you run dune build @fmt --auto-promote)?

Hey @pitag-ha, I actually missed doing that. I'll run that now!

you can rebase your PR over my branch to see if that solves the problem

Yes, I haven't done that before, I'll sure try that.

And just one detail about the code: as already said, it looks very good! One thing that might be nice improving would be to factor out the creation of the error extension node into a separate function and use that function at the places where the error occurs. What do you think?

Yes, I do think that would be really effective allowing us to reuse the code. I'll try to think about how to bring about that change.

Anu-123-gif commented 2 years ago

I agree it looks good! I would add that it might be worth it to also remove the Raise functions now that they are not used, and to also include the updated tests to the pr.

Yeah, I totally agree, I haven't removed them as some of the errors are still using them right now. After embedding error_extensionf for them as well they will become redundant. And I'll work on adding testing as well. Thanks!

Anu-123-gif commented 2 years ago

here seems to be a problem installing ocamlformat in the CI (ocamlformat is the formatter for OCaml). btw, have you formatted your code (i.e. have you run dune build @fmt --auto-promote)? I'm just asking because the CI currently can't check that...

I ran the command but I found Error: Program ocamlformat not found in the tree or in PATH (context: default) -> required by _build/default/test/rewriter/errors/.formatted/expr_anti_quotation_payload.ml -> required by alias test/rewriter/errors/.formatted/fmt -> required by alias test/rewriter/errors/fmt I guess there is an issue. I would love your help in solving it. Thanks!

pitag-ha commented 2 years ago

Hey @Anu-123-gif , to format your code, you first need to install ocamlformat.0.19.0 . Do you already know how to install Ocaml packages? Or do you want me to give more detail?

Anu-123-gif commented 2 years ago

Hey @Anu-123-gif , to format your code, you first need to install ocamlformat.0.19.0 . Do you already know how to install Ocaml packages? Or do you want me to give more detail?

Hey @pitag-ha I will Google this up and I'll ask if I have doubts .

I had a request btw, I have an exam tomorrow so I will not be able to work on this PR for a few hours . So could you please consider my contribution tomorrow as well. I'll definitely finish work on this PR tomorrow positively. I would be really grateful if we could make this work. I have already put a lot of effort in it, I would love to take it to fruition.

NathanReb commented 1 year ago

Thanks for taking the time to work on this!

Are you interested in picking this up? The PR can't be merged in its current state and I unfortunately can't push changes to your upstream branch to wrap it up quickly.

Please let me know if you'd like to finish this otherwise I'll have to close this to re-open a separate PR myself. Either options are fine by me so don't feel pressured into finishing it if you don't want to!

NathanReb commented 1 year ago

I'm closing this in favor of #44