That looks better. My other comments still need to be addressed. You should also make sure the patch applies cleanly to a fresh repo. I think I saw some whitespace problems.
It will be easier to review if you can figure out how to get git-send-email to work. On Fri, Feb 11, 2022 at 10:43 AM Shashvat <shashvatjain2...@gmail.com> wrote: > > Here is the patch, I ran yapf on it and it didn't introduce any changes. > Please take a > look and let me know what you think. > > On Fri, 11 Feb 2022, 10:31 pm Gedare Bloom, <ged...@rtems.org> wrote: >> >> On Fri, Feb 11, 2022 at 9:38 AM Shashvat <shashvatjain2...@gmail.com> wrote: >> >> >> >> >> > >> > Hi Gedare!! >> > >> >> Is there a ticket associated with this, or any feature request? Or >> >> just something you thought of doing? >> > >> > >> > I am sorry I should have mentioned the motive behind the option. >> > I was planning to work on ticket #3333 which works on a specific >> > posix-users manual afaik. I wanted waf to build only this particular >> > manual so asked Chris on discord if it is possible and he told me how it >> > was broken, and that it would be good to add an option that enables this. >> >> >> >> >> >> I don't think an extra blank line is needed here. >> > >> > >> > This was my fault. >> >> >> >> > >> >> > $ ./waf >> >> > >> >> > @@ -448,8 +450,10 @@ verbose level: >> >> > $ ./waf configure --sphinx-options "-V -V" >> >> > $ ./waf clean build >> >> > >> >> > -You can enter a manual's directory and run the same configure command >> >> > and >> >> > build >> >> > -just that manual. >> >> > +If you wish to build only some specific manuals, >> >> > +use the '--build-manuals=<manual-name-1>,<manual-name-2>' option with >> >> > +configure to build only those specific manuals. >> >> > + >> >> > >> >> > Documentation Standard >> >> > ---------------------- >> >> > diff --git a/common/waf.py b/common/waf.py >> >> > index fa9aecb..e6ae059 100644 >> >> > --- a/common/waf.py >> >> > +++ b/common/waf.py >> >> > @@ -240,6 +240,11 @@ def cmd_configure(ctx): >> >> > check_sphinx_extension(ctx, 'sphinxcontrib.bibtex') >> >> > >> >> > # >> >> > + # Build specific manuals. >> >> This spacing looks wrong. >> >> >> >> > + # >> >> > + if ctx.options.build_manuals!="": >> >> Follow the coding style of the surrounding text. For Python code, we >> >> generally follow >> >> https://docs.rtems.org/branches/master/eng/python-devel.html >> > >> > >> > Thanks, I will take a look. >> >> >> >> >> >> > + ctx.env.MANUALS = ctx.options.build_manuals.split(',') >> >> Probably want a blank line here. None of the other options take >> >> multiple values. I wonder if there is any value to having a multiple >> >> option here, versus building just one manual selectively? You could >> >> still have an 'all' option as the default. That will reduce the >> >> complexity of the command line argument processing. >> > >> > >> >> >> >> I guess by default this will not build any manuals? The principle of >> >> least surprise suggests that by default the behavior should be what it >> >> used to be if you omit the argument, so build everything. otherwise, >> >> you break existing workflows and scripts. >> > >> > >> > This does has the action of building all manuals by default. >> >> >> >> >> >> This is not good python, no indent after 'if', so there's nothing in >> >> the conditional code block, you just always set building to >> >> ctx.env.MANUALS. >> >> >> >> > + print("Building the following manuals:-") >> >> > + for manual in building: >> >> > + print(manual) >> >> missing indent here too. But the print statements seem to be >> >> inconsistent with other printed output for this code. You generally >> >> want to keep that consistent. >> > >> > >> > This is my first time submitting a patch by mail and looks like I messed >> > something up up while copying the diff, should have checked by applying >> > the diff before submitting. Looks like the indents are missing :) >> > >> If you don't have git-send-email working, then use git-format-patch >> and just email the patch instead of trying to copy-paste into your >> emailer >> >> > >> > Thanks >> > Shashvat _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
