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
Support custom directories to watch when running mkdocs serve #2642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Seems good overall.
Co-authored-by: Oleh Prypin <oleh@pryp.in>
mkdocs/config/defaults.py
Outdated
@@ -120,4 +120,7 @@ def get_schema(): | |||
# A key value pair should be the string name (as the key) and a dict of config | |||
# options (as the value). | |||
('plugins', config_options.Plugins(default=['search'])), | |||
|
|||
# a list of extra directories to watch while running `mkdocs serve` | |||
('watch', config_options.Type(list, default=[])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the path will have to be relativized to the location of mkdocs.yml
, as here:
mkdocs/mkdocs/config/config_options.py
Lines 369 to 370 in 9880767
if self.config_dir and not os.path.isabs(value): | |
value = os.path.join(self.config_dir, value) |
Well, I think that's what would be the most logical as per what I've seen from MkDocs so far.
Feel free to argue otherwise.
This would probably require a new type in config_optons.py. ListOfPaths
or something.
During validation it could reuse FilesystemObject
.
I would recommend turning it non-abstract:
class FilesystemObject(Type):
+ existence_test = staticmethod(os.path.exists)
+ name = 'path'
Relevant implementation example:
mkdocs/mkdocs/config/config_options.py
Line 494 in 9880767
class Nav(OptionallyRequired): |
Note that we probably should not check for existence of the path (exists=False
should be kept). Not sure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: we should, in fact, check for existence (just set exists=True
), otherwise it throws an OSError
anyway after startup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify why this is important:
watch:
- foo
mkdocs serve --config-file=subdir/mkdocs.yml -w bar
I want this to watch the foo
that is adjacent to mkdocs.yml
.
The difference in the output being:
-Watching paths for changes: 'subdir/docs', 'subdir/mkdocs.yml', 'foo', 'bar'
+Watching paths for changes: 'subdir/docs', 'subdir/mkdocs.yml', 'subdir/foo', 'bar'
(-w
flag not relativized intentionally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving this to you btw.
And some tests for the new config option would be nice :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
root@08e90cd25d84:/# mkdocs serve --config-file=mkdocs/mkdocs.yml -w home
INFO - Building documentation...
INFO - Cleaning site directory
INFO - Documentation built in 1.09 seconds
INFO - [01:17:16] Watching paths for changes: 'mkdocs/docs', 'mkdocs/mkdocs.yml', 'mkdocs/foo', 'home'
seems to be working :).
agree this needs some tests. i'll have that soon. might take a day or two to carve out some more time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit tests for ListOfPaths
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll argue in favor of this 🙂
As a data point, see this mkdocstrings issue.
That ListOfPaths
type could be very useful to resolve it!
Report watched directories in one line, move code to livereload
Co-authored-by: Oleh Prypin <oleh@pryp.in>
Co-authored-by: Oleh Prypin <oleh@pryp.in>
I'm not sure if/where it is stated in the docs that paths in the config file are relative to the config file itself. Maybe this could be added again in the docs for the watch option, and/or in the changelog, to make it clear? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Co-authored-by: Oleh Prypin <oleh@pryp.in>
Co-authored-by: Oleh Prypin <oleh@pryp.in>
Co-authored-by: Oleh Prypin <oleh@pryp.in>
Co-authored-by: Oleh Prypin <oleh@pryp.in>
…into 2632-watch
nice catch @pawamoy. I've updated the docs with a note admonition calling this out. |
Thank you! |
|
||
The paths provided via the configuration file are relative to the configuration file. | ||
|
||
The paths provided via the `-w`/`--watch` CLI parameters are not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit late to the party, but what exactly would it then be? What would it's "position" be based of? Where the command is executed? That should've been mentioned here IMO, so maybe in a future PR/Commit could this be changed?
Implements #2632
watch
property to themkdocs.yaml
schema. Accepts a list of directories to watch.-w
/--watch
command line option tomkdocs serve
that can be passed multiple timesmkdocs.yaml
and CLI flags are combined