Remove logic around finding a configuration file inside directories #4714
Conversation
|
Thanks alot @stsewd for reviewing. I wanted to know that if find_all is removed should the tests check number of files or check if there are multiple files? |
|
@sriks123 I don't fully understand your question, but new tests should only find one top level file, no more finding files inside directories. |
|
@stsewd can you please review the changes that I made ? |
|
|
||
| from .utils import apply_fs | ||
|
|
||
|
|
||
| def test_find_no_files(tmpdir): | ||
| with tmpdir.as_cwd(): | ||
| paths = list(find_all(os.getcwd(), r'readthedocs.yml')) | ||
| paths = str(find_one(os.getcwd(), r'readthedocs.yml')) | ||
| assert len(paths) == 0 |
There was a problem hiding this comment.
We need to change this to test that find_one returns None
| str(tmpdir.join('first', '.readthedocs.yml')), | ||
| str(tmpdir.join('third', 'readthedocs.yml')), | ||
| } | ||
| paths = find_one(base, r'readthedocs\.yml') |
| ] | ||
|
|
||
|
|
||
| def test_find_nested(tmpdir): |
There was a problem hiding this comment.
I'd left this test to check that we are not finding nested files
| str(tmpdir.join('third', 'readthedocs.yml')), | ||
| } | ||
| paths = find_one(base, r'readthedocs\.yml') | ||
| assert paths == str(os.path.abspath(os.path.join(base, 'readthedocs.yml'))) |
readthedocs/config/find.py
Outdated
| _path = os.path.abspath(path) | ||
| for filename in os.listdir(_path): | ||
| if re.match(filename_regex, filename): | ||
| return os.path.abspath(os.path.join(_path, filename)) |
There was a problem hiding this comment.
I think we only need the asbpath of filename, no need to join it with _path or just join _path and filename (path is already an absolute path).
|
@stsewd I made some changes as you suggested I am working on including on that extra case can you please review this ? |
| @@ -94,6 +94,22 @@ def test_load_no_config_file(tmpdir, files): | |||
| base = str(tmpdir) | |||
| with raises(ConfigError) as e: | |||
| load(base, env_config) | |||
| nested_files = { | |||
| 'first': { | |||
There was a problem hiding this comment.
This case should be moved to the decorator, not inside the function
| @@ -1089,12 +1098,12 @@ def test_python_requirements_default_value(self): | |||
| assert build.python.requirements is None | |||
|
|
|||
| def test_python_requirements_allow_null(self): | |||
| build = self.get_build_config({'python': {'requirements': None}},) | |||
| build = self.get_build_config({'python': {'requirements': None}}, ) | |||
There was a problem hiding this comment.
These modifications don't seem to belong to this PR
| paths = list(find_all(os.getcwd(), r'readthedocs.yml')) | ||
| assert len(paths) == 0 | ||
| paths = str(find_one(os.getcwd(), r'readthedocs.yml')) | ||
| assert paths == '' |
There was a problem hiding this comment.
No need to call str here, and we should rename the var name to just path
Fixes issue: #4669