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

fix: search file by supported extension use manually specific config … #1840

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AuZhoomLee
Copy link

this PR is aimed to resolve issue #1754 and #1401, with compatible changes about searchInPath

test all imaginable situation passed and statement coverage is 100%:

=== RUN   Test_searchInPath
=== RUN   Test_searchInPath/find_file_without_extension
=== RUN   Test_searchInPath/cannot_find_file_with_extension
=== RUN   Test_searchInPath/find_file_with_supported_extension
=== RUN   Test_searchInPath/find_file_with_specific_extension_from_multiple_supported_files
=== RUN   Test_searchInPath/find_file_with_unsupported_extension
--- PASS: Test_searchInPath (0.00s)
    --- PASS: Test_searchInPath/find_file_without_extension (0.00s)
    --- PASS: Test_searchInPath/cannot_find_file_with_extension (0.00s)
    --- PASS: Test_searchInPath/find_file_with_supported_extension (0.00s)
    --- PASS: Test_searchInPath/find_file_with_specific_extension_from_multiple_supported_files (0.00s)
    --- PASS: Test_searchInPath/find_file_with_unsupported_extension (0.00s)
PASS
coverage: 17.1% of statements in ./...

@CLAassistant
Copy link

CLAassistant commented May 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

👋 Thanks for contributing to Viper! You are awesome! 🎉

A maintainer will take a look at your pull request shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news,
either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

@AuZhoomLee
Copy link
Author

coverage show:

viper searchfile pt

@AuZhoomLee
Copy link
Author

could you review it @sagikazarmark

refer to #1795

@sagikazarmark
Copy link
Collaborator

Although I see the value in this change, I'm afraid it would create more confusion later on.

I'm working on #1849 right now. I hope it will help people make configuration searching easier.

@AuZhoomLee
Copy link
Author

AuZhoomLee commented Jun 3, 2024

#1849 is perfer good design, BUT the orginal function of SetConfigType means specific the type of file config, it's not functional as the name and comments mention, which is:

// SetConfigType sets the type of the configuration returned by the
// remote source, e.g. "json".

why not let it work as expectly? it's now works in some way, like if no other supported file name exist in the current file path.

em... , I think it's more confused to maintain this issue of this function.

SO, I want to 2 points:

  1. the change I committed realise worked seems no other issue, or can u give me another unexpectd example instance?
  2. if the function have certainly issue about this, why don't mark it Depracated, and supply more poper way to replace this.

@sagikazarmark
Copy link
Collaborator

SetConfigType was supposed to override the detected file type (or set it for remote kv stores). It wasn't supposed to be used for configuring file search.

So when you say it's the expected functionality, it's not exactly true, although I understand why people would expect that.

I don't have an exact example that would break, but given the complexity (and past experiences) it could easily break for someone out there. Then what?

Let me think about it. But right now, I'd prefer deprecating the old API and drive people towards the new one.

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.

3 participants