Check for readthedocs file with yaml extension #48

StefanoChiodino · 2018-05-22T20:58:32Z

Fixes readthedocs/readthedocs.org#4102.
Ref. readthedocs/readthedocs.org#4129 that documents this change.

Fix some typos and document the new file extension.

stsewd · 2018-05-22T21:04:47Z

readthedocs_build/config/config.py

@@ -14,7 +14,7 @@
    'load', 'BuildConfig', 'ConfigError', 'InvalidConfig', 'ProjectConfig')


-CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml')
+CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml', 'readthedocs.yaml')


We need a .readthedocs.yaml too. Maybe we can refactor this to be a regex?

Good point, I was considering even an helper method as this is not the only yaml file in use.

Done, notice that now also .readthedocs.yaml is allowed. If you are happy with this implementation I'll document it but I'm not sure that listing 4 exact filenames with a slight difference is very human friendly, so I was thinking something along the lines of
... readthedocs.yml (optionally with . prefix, or yaml extension)

Most importantly: functionally it's now not possible to specify which file to prefer. There was a test that checked that files were found in the correct order (e.g. the order specified in the list), I had to reduce it to just testing whether it could find the files.

StefanoChiodino · 2018-05-23T06:20:43Z

There's a test that is failing on travis but not on my pc. I think I'll have to change that list to a set anyway. I'll check it later.

StefanoChiodino · 2018-05-23T07:05:38Z

Weirdly enough 18 tests fails for me on master, that's weird.

stsewd · 2018-05-23T21:06:47Z

I think I'll have to change that list to a set anyway

Yeah, that should be a set.

StefanoChiodino · 2018-05-25T15:24:48Z

@stsewd Is this good, or should I change the description to read something like
readthedocs.yml (with optional '.' prefix or 'yaml' file extension)
??

StefanoChiodino · 2018-06-05T05:39:35Z

Any luck with this?

stsewd · 2018-06-05T05:46:09Z

@StefanoChiodino I'll try to review this in the morning

stsewd

I left some minor recommendations

stsewd · 2018-06-05T16:14:04Z

readthedocs_build/config/test_find.py

@@ -39,11 +39,11 @@ def test_find_nested(tmpdir):
    apply_fs(tmpdir, {'first/readthedocs.yml': ''})

    base = str(tmpdir)
-    paths = list(find_all(base, ('readthedocs.yml',)))
-    assert set(paths) == set([
+    paths = set(list(find_all(base, r'readthedocs\.yml')))


We can omit the list conversion here and directly do a set

stsewd · 2018-06-05T16:15:29Z

readthedocs_build/config/test_find.py

-    paths = list(find_all(base, ('readthedocs.yml',
-                                 '.readthedocs.yml')))
-    assert paths == [
+    paths = set(list(find_all(base, r'\.?readthedocs\.yml')))


stsewd · 2018-06-05T16:20:59Z

integration_tests/minimal_project/readthedocs.yaml

@@ -0,0 +1,2 @@
+name: docs


Looks like we aren't using this file, do we?

If you look at https://github.com/rtfd/readthedocs-build/pull/48/files#diff-c5258b81d7a5c3182be62b7f82e02aedR105 there is a new test that uses it. Not sure if this is redundant considering the "find" functionality tests.

The apply_fs function takes the dict and creates a filesystem with dirs and files on a temporal location.

So, the files inside integration_tests/ are used in other tests

@stsewd maybe I'm confused: I'm thinking that the apply_fs function makes a temporary directory to test, and because I'd like to test whether the .yaml file is found I've added readthedocs.yaml to this folder and created the test to take this file and see if it's found. Am I missing something? Thanks

I've added readthedocs.yaml to this folder

If you mean by this line
https://github.com/rtfd/readthedocs-build/pull/48/files#diff-c5258b81d7a5c3182be62b7f82e02aedR52

Then, the file that you created isn't necessary, the apply_fs will create that file for you in a temporary directory with the content name: docs\ntype: sphinx. The file in minimal_project isn't necessary for this test.

Thanks for explaining! I'll give it a quick try pre-work :)

@stsewd tests passes :)

stsewd

I leave just a thought, otherwise LGTM

stsewd · 2018-06-08T14:45:23Z

readthedocs_build/config/config.py

@@ -14,7 +14,7 @@
    'load', 'BuildConfig', 'ConfigError', 'InvalidConfig', 'ProjectConfig')


-CONFIG_FILENAMES = ('readthedocs.yml', '.readthedocs.yml')
+CONFIG_FILENAME_REGEX = r'\.?readthedocs.ya?ml'


I wonder if isn't better to test this regex (because right now we are hardcoding a regex in the tests)

Sure, can do that too, is a very easy test, I'm just wondering what will I actually be testing? I can see 4 permutations that the regex need to test, but should I test that it doesn't match something else as well?

I think testing matching is enough

StefanoChiodino · 2018-06-09T14:05:39Z

@stsewd happy with this?

stsewd

Thanks

agjohnson

Thanks for the contribution! I think the additional filenames make sense. We could also explore adding json to this list, as Python YAML will load json files fine. That's later though.

I think the changes here look good, but we're currently in the middle of moving this code to rtd core. I'll set this as blocking for now, but once the porting is finished, we should redo this PR as a pr against rtd core.

agjohnson · 2018-06-19T17:46:02Z

readthedocs_build/config/config.py

-            files += ' or '
-        files += '{!r}'.format(CONFIG_FILENAMES[-1])
-        raise ConfigError('No files {} found'.format(files),
+        raise ConfigError('No files with regex \'{}\' found'.format(CONFIG_FILENAME_REGEX),


We don't need to mention regex here as this is a user facing error. The error message should probably just be "No configuration file found."

agjohnson · 2018-08-03T02:54:04Z

Okay, this is now unblocked. As noted, we are moving this logic to rtfd/readthedocs.org repository. The next step is going to be replicating these changes in that repo. Sorry this is probably an annoying request, I don't know the best way to port this over easily. A diff might get most of the way there if you are able to put the change in soon. Once the code there deviates, this PR is going to need a manual rewrite, likely.

StefanoChiodino · 2018-08-13T18:48:54Z

Done, hopefully (tests aren't playing nice on my machine) readthedocs/readthedocs.org#4512

stsewd · 2018-09-05T17:34:30Z

Ported on readthedocs/readthedocs.org#4512

StefanoChiodino added 3 commits May 22, 2018 21:30

Test for yaml config file extension (fails).

f044969

Add a basic .yaml file and add it to the config filenames.

9c000fb

Reduce test to just being able to load the file.

f1fa2db

Fix some typos and document the new file extension.

StefanoChiodino mentioned this pull request May 22, 2018

Document that readthedocs file can now have yaml extension readthedocs/readthedocs.org#4129

Merged

stsewd reviewed May 22, 2018

View reviewed changes

Use regex to find the readthedocs yaml file.

d4c888b

Use sets for paths found now that order doesn't matter any more.

d87677c

stsewd reviewed Jun 5, 2018

View reviewed changes

StefanoChiodino added 2 commits June 5, 2018 20:43

Convert directly to sets

887e0ea

Remove .yaml file that was not actually necessary.

cc2ef31

stsewd approved these changes Jun 8, 2018

View reviewed changes

StefanoChiodino added 3 commits June 9, 2018 08:55

Add test for possible config filenames.

0f1f4a6

Improve code readability

ec96e3b

Improve file naming options

afffb2e

stsewd approved these changes Jun 9, 2018

View reviewed changes

agjohnson suggested changes Jun 19, 2018

View reviewed changes

agjohnson added the Status: blocked Issue is blocked on another issue label Jun 19, 2018

Make config error more generic.

1b1ae9a

agjohnson removed the Status: blocked Issue is blocked on another issue label Aug 3, 2018

StefanoChiodino mentioned this pull request Aug 13, 2018

Allow for period as a prefix and yaml extension for config file readthedocs/readthedocs.org#4512

Merged

stsewd closed this Sep 5, 2018

Check for readthedocs file with yaml extension #48

Check for readthedocs file with yaml extension #48

Uh oh!

Conversation

StefanoChiodino commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanoChiodino commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StefanoChiodino commented May 23, 2018

Uh oh!

stsewd commented May 23, 2018

Uh oh!

StefanoChiodino commented May 25, 2018

Uh oh!

StefanoChiodino commented Jun 5, 2018

Uh oh!

stsewd commented Jun 5, 2018

Uh oh!

stsewd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stsewd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanoChiodino commented Jun 9, 2018

Uh oh!

stsewd left a comment

Choose a reason for hiding this comment

Uh oh!

agjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agjohnson commented Aug 3, 2018

Uh oh!

StefanoChiodino commented Aug 13, 2018

Uh oh!

stsewd commented Sep 5, 2018

Uh oh!

StefanoChiodino commented May 22, 2018 •

edited

Loading

StefanoChiodino commented May 23, 2018 •

edited

Loading