Fix #8: Incremental building not working #14
Conversation
|
Thank you, at first glance it looks good. I have however not dealt with the module configuration before so I will need some time to replicate your findings from Issue #8 and review the changes proposed in this PR. I plan on having it done this week. Do you think we should make a new release with this fix? |
|
I think it could be a good idea. I have no plan to code during next weeks for this project (I need more time...). |
There was a problem hiding this comment.
@jonascj this PR looks like it fixes the issue for me too. Looking at Sphinx itself, it checks if the project needs to be rebuilt when it creates the env specifically in BuildEnvironment._update_config which is called by BuildEnvironment.setup which happens between the events "config-inited" and "builder-inited" and is why any config needs to happen earlier than it is already.
| def configure_backend(app, main_config): | ||
| global DEFAULT_CONFIG | ||
|
|
||
| config = copy.deepcopy(DEFAULT_CONFIG) | ||
| config.update(app.config.images_config) | ||
| app.config.images_config = config |
There was a problem hiding this comment.
I would probably use the main_config directly since it's provided, but I'm not sure if there is a good reason why the hook has both app and the config though.
| def configure_backend(app, main_config): | |
| global DEFAULT_CONFIG | |
| config = copy.deepcopy(DEFAULT_CONFIG) | |
| config.update(app.config.images_config) | |
| app.config.images_config = config | |
| def check_config(app, config): | |
| '''Ensure all config values are defined''' | |
| merged = copy.deepcopy(DEFAULT_CONFIG) | |
| merged.update(config.images_config) | |
| config.images_config = merged |
| global DEFAULT_CONFIG | ||
|
|
||
| config = copy.deepcopy(DEFAULT_CONFIG) | ||
| config.update(app.config.images_config) | ||
| app.config.images_config = config | ||
|
|
||
| def builder_init(app): |
There was a problem hiding this comment.
This is still configuring the backend and probably shouldn't be renamed
| def builder_init(app): | |
| def configure_backend(app): |
| app.connect('config-inited', configure_backend) | ||
| app.connect('builder-inited', builder_init) |
There was a problem hiding this comment.
Based on the suggestions above:
| app.connect('config-inited', configure_backend) | |
| app.connect('builder-inited', builder_init) | |
| app.connect('config-inited', check_config) | |
| app.connect('builder-inited', configure_backend) |
| @@ -346,7 +348,8 @@ def setup(app): | |||
| global DEFAULT_CONFIG | |||
There was a problem hiding this comment.
This global declaration isn't needed, and it might be good to remove it while you're editing this file
| @@ -346,7 +348,8 @@ def setup(app): | |||
| global DEFAULT_CONFIG | |||
| app.require_sphinx('1.0') | |||
| app.add_config_value('images_config', DEFAULT_CONFIG, 'env') | |||
There was a problem hiding this comment.
Since this isn't actually needed because the configuration will be merged anyways, it might make sense to just pass a dict which also wouldn't break when copying it.
| app.add_config_value('images_config', DEFAULT_CONFIG, 'env') | |
| app.add_config_value('images_config', {}, 'env') |
|
How time flies when you have a baby and a 3-year-old! Please accept my apologies for not attending to the PRs and issues in a timely fashion. I'll attend to them at the latest on August 29th and 30th (I have another project with deadline on August 28th). |
@jonascj not sure @kujiu has had a chance to review my suggestions, but if you'd like me to add my suggestions to another branch and create a PR off of that I can do so. I'd leave attribution for @kujiu so they'll get credit for their work. |
|
Sorry, I didn't see the notification for comments. I don't have time this week and perhaps the next too, but I don't care about credits. You can do the stuff and erase my contribution if you want. Sorry, I have really big projects. |
|
@terencehonles I just meant I'll go over the PR/issue discussions, start reviewing contributions etc. I have no desire to redo good work already done. So if @kujiu has found a good, viable solution we'll use it and credit him one way or another, don't worry. |
No worries @kujiu , I wasn't sure if you were checking your github notifications often. I updated the commit here: terencehonles@f1184cd You can quickly update this PR with the following commands: # fetch from my repo without adding another remote
git fetch [email protected]:terencehonles/sphinx-contrib-images.git master-fix-incremental-build
# take what you just fetched and push it to the branch this PR is tracking
git push origin +FETCH_HEAD:master-fix-incremental-build
@jonascj the suggestions are not re-doing anything, just clarifying what the plugin is doing with respect to the changes in the PR |
|
Any progress? |
|
@terencehonles Thank you for moving it forward, I'll try to look at it this coming weekend (9th, 10th January). @SilverRainZ When you asked there was no new progress, no, but now there is a little progress. See also #20 :-) |
|
sorry I had too many tabs open and looks like I put it on the wrong one 🤦 it's fixed now |
Changing config parameters after config provokes regenerating all files. This PR update config parameters during
config-initedevent.