Skip to content

Conversation

@jamesbraza
Copy link
Collaborator

@jamesbraza jamesbraza commented Aug 16, 2025

This PR helps with #1044

  • Shows lookup locations
  • Doesn't clobber user config location if name collision

Now the FileNotFoundError looks like:

> pqa --settings hi ask "..."
FileNotFoundError: No configuration file 'hi' found at user config path /Users/jamesbraza/.pqa/settings/hi.json or bundled config path /Users/jamesbraza/code/paper-qa/src/paperqa/configs/hi.json.

@jamesbraza jamesbraza self-assigned this Aug 16, 2025
Copilot AI review requested due to automatic review settings August 16, 2025 19:33
@jamesbraza jamesbraza added the bug Something isn't working label Aug 16, 2025
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 16, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves error messaging for configuration file lookup failures in Settings.from_name by showing both lookup locations and preventing the clobbering of user config paths when name collisions occur.

  • Reorganizes the config file lookup logic to calculate both user and package paths upfront
  • Enhances the FileNotFoundError message to include both the user config path and bundled config path
  • Fixes the import statement to use the correct module reference

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jamesbraza jamesbraza force-pushed the better-not-found-message branch from 80792ce to 839e2da Compare August 16, 2025 19:33
@jamesbraza jamesbraza force-pushed the better-not-found-message branch from 839e2da to e23583a Compare August 16, 2025 19:35
raise FileNotFoundError(
f"No configuration file found for {config_name}"
) from e
else:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously without this else we would overwrite the user_config_path location -- @mskarlin does this seem like a bug to you?

Copy link
Collaborator

@whitead whitead left a comment

Choose a reason for hiding this comment

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

nice bugfix

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 16, 2025
@jamesbraza jamesbraza merged commit 2d58202 into main Aug 16, 2025
5 checks passed
@jamesbraza jamesbraza deleted the better-not-found-message branch August 16, 2025 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants