[Fix pipeline] multiple_kernels.rst must respect doctor-rst #18295
Conversation
|
Not sure about the directory. I mean maybe we should keep/allow apps here. I would not change the director structure just to make a tool happy, but if it doesn't matter and it is just an example and no best practice, we can rename it to applications directory |
|
👎 on my side, the current names are better to me. |
|
I reverted For doctor-rst, I think exclude all file is not good because it will hide other violations. I created PR OskarStark/doctor-rst#1395 in order to exclude some rules by file when is needed |
|
Antoine, thanks for fixing this and for the updates after the reviewers comments. |
…rst (alamirault) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Fix pipeline] multiple_kernels.rst must respect doctor-rst #18186 (comment) `apps` -> `applications` `ContainerConfigurator $container` -> `ContainerConfigurator $containerConfigurator` Commits ------- 07f6609 [Fix pipeline] multiple_kernels.rst must respect doctor-rst
240ae8d to
07f6609
Compare
|
Note: GitHub shows this as closed, but it was merged. See e562839 |
|
I wouldn't have merged the change of var name, it's not consistent with the name in the recipe we used at the time: nor with the name in the trait we use now: and it's longer without adding much. I'd propose reverting. Sorry... |
|
@nicolas-grekas yes, I see that there's a minor inconsistency between the code and the docs about this variable name. We decided to make that change in the past (in the PRs mentioned by @alamirault) to make docs as self-explanatory and clear as possible. Ideally, we'd like to make the same change in the code ... but we know that for code is not that important and also, changing the name in code is more troublesome than in docs. Thanks. |
Sorry, something went wrong.
|
$containerConfigurator is not the name we recommend in config files either, see eg I see no need for consistency between those two examples. |
|
Sorry for the take but I would be fine reverting #17683 also. The previous contributors thoughts $container was an appropriate name, and I still agree with them today, no need for the extra word in the name IMHO. |
This PR was merged into the 5.4 branch. Discussion ---------- Use "$container" consistently Takes the other road from #17683 as discussed in #18295 Using `$containerBuilder` and `$containerConfigurator` is unnecessarily long, consuming horizontal space without adding any clarity. This als 69FA o aligns the name `$container` with existing practice in the code and in recipes. Commits ------- 26d021c Use "$container" consistently
#18186 (comment)
apps->applicationsContainerConfigurator $container->ContainerConfigurator $containerConfigurator