-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Improve error message when src directory for repo add doesn't exist #5689
Conversation
Thanks for the PR. On the general idea, lgtm! |
There are some existing tests which already fail because of this change. I will go ahead and fix them, to begin with. |
I've added a commit that contains the code review & test update. |
Thanks for the code improvements and the test fixes, @rjbou ! |
2cb75c1
to
b088191
Compare
I've updated the PR, a review from @kit-ty-kate and it's good to merge! |
Sorry for the delay, we are focusing on the release, and there is only me and partly kate working on opam. |
Thank you for your work on opam! No worries at all -- I'm not in a hurry to have this merged. I just didn't want to leave it hanging there, and wanted clarity on whether it's waiting on me. |
b088191
to
87c1063
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to #6027 I think the errors are less readable. A good error message already exist in the data (the second value of the pair) and duplicating it seems superfluous.
Closes ocaml#5632 Co-authored-by: Raja Boujbel <raja.boujbel@ocamlpro.com>
87c1063
to
f02cdd4
Compare
Closing in favor of #6027. Thank you for your proposal! |
Closes #5632