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

Introduce an option for disabling the automatic resolver in FileSourceFile #133

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eisterman
Copy link
Contributor

A new feature with granted retro compatibility.
To solve the problem introduced in #132 I have introduced the solution number 3:

Add a flag to the FileSourceFile struct named use_resolver and add a method in File<FileSourceFile> similar to required(bool) where the user can enable or disable the resolver in FileSourceFile (The best solution. No breaking change, no removing of feature, only adding an option flag that if not used will be set to True for retro compatibility)

The struct File now has two new methods, with_exact_name and exact_name, the first to create a File with a FileSourceFile imposed to not search the extension of the file.
With this approach, one can load a file named Settings.wrongextension as a Toml file like before, but if the file is missing the program will not search another file named for example Settings.yaml without asking.

I've introduced three tests for the new methods, and a very small fix of the wrong extension error message, because before it used the absolute path when the not found error used the relative/given path.

@mehcode
Copy link
Collaborator

mehcode commented Mar 15, 2020

I see where this is coming from. First, thanks for the contribution. I agree as well that your solution #3 is probably the best way to handle this.

Very minor nit, should we have the option method simply exact instead of exact_name? I think it conveys the same information in context:

File::with_name("Config.toml").exact(true)
File::with_exact_name("Config.toml")

@eisterman
Copy link
Contributor Author

I agree it is certainly more coherent and sensible as a name.
I sent the name change commit.

@eisterman
Copy link
Contributor Author

Do you need anything else for the merge?

@eisterman
Copy link
Contributor Author

Any problems?

@matthiasbeyer
Copy link
Member

Are you still interested in this?

If yes, I created a maintenance fork (read here).
Feel free to submit your patches! 😄

@matthiasbeyer
Copy link
Member

Can you rebase this to latest master?

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