Skip to content

Conversation

@Anu-123-gif
Copy link

@Anu-123-gif Anu-123-gif commented Apr 21, 2022

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
Copy link
Contributor

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:

  • there 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...
  • and the other one is actually on me and should be solved by a PR I've just opened Update tests to ppxlib.0.26.0 #36. If you want to, you can rebase your PR over my branch to see if that solves the problem (but if you aren't very familiar with git, no need to do so).

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
Copy link
Contributor

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
Copy link

ayc9 commented Apr 21, 2022

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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 NathanReb self-requested a review April 22, 2022 13:26
@NathanReb
Copy link
Owner

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
Copy link
Owner

I'm closing this in favor of #44

@NathanReb NathanReb closed this Oct 3, 2023
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.

4 participants