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

clarify error message on missing path #4278

Closed
wants to merge 1 commit into from
Closed

clarify error message on missing path #4278

wants to merge 1 commit into from

Conversation

H4vC
Copy link

@H4vC H4vC commented Jun 21, 2024

fix for #4277

@H4vC H4vC closed this Jun 21, 2024
@H4vC H4vC reopened this Jun 21, 2024
@H4vC
Copy link
Author

H4vC commented Jun 21, 2024

This is a trivial change.

@davidhewitt
Copy link
Member

davidhewitt commented Jun 21, 2024

Thanks for the PR! We actually have #4043 also improving this line, but that got stuck in review limbo. Would you be willing to help finish that one off instead please? You could pull those commits and add some extra ones on top.

The changes waiting on that PR were:

  • how to test it - just write a unit test that attempts to read from a nonexistent file at the bottom of the same file
  • some suggestions to actually include PYO3_CROSS_LIB_DIR in the error message

@mrexodia
Copy link

Thanks for the PR! We actually have #4043 also improving this line, but that got stuck in review limbo. Would you be willing to help finish that one off instead please? You could pull those commits and add some extra ones on top.

The changes waiting on that PR were:

  • how to test it - just write a unit test that attempts to read from a nonexistent file at the bottom of the same file
  • some suggestions to actually include PYO3_CROSS_LIB_DIR in the error message

This change as-is would have saved me a lot of time already. Considering it's a 1-line change wouldn't it be good to merge this as-is and then refactor later?

@davidhewitt
Copy link
Member

I completely agree and am sorry that this wasn't fixed sooner. Nobody wants time wasted by bad error messages.

I found a moment to push to #4043 just now, so let's merge that one. Thank you both for reminding me that this error message matters. (You were not the first folks to hit this case.)

@mrexodia
Copy link

Awesome, thanks for the quick fix!

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

3 participants