Re: [PATCH] dts: improve documentation

[Thomas Monjalon <thomas@monjalon.net>]

04/01/2024 13:34, Luca Vizzarro:
> On 04/01/2024 10:52, Thomas Monjalon wrote:
> >>   DTS needs to know which nodes to connect to and what hardware to use on those nodes.
> >> -Once that's configured, DTS needs a DPDK tarball and it's ready to run.
> >> +Once that's configured, DTS needs a DPDK tarball or a git ref ID and it's ready to run.
> > 
> > That's assuming DTS is compiling DPDK.
> > We may want to provide an already compiled DPDK to DTS.
> 
> Yes, that is correct. At the current state, DTS is always compiled from 
> source though, so it may be reasonable to leave it as it is until this
> feature may be implemented. Nonetheless, my change just informs the user
> of the (already implemented) feature that uses `git archive` from the 
> local repository to create a tarball. A sensible change would be to add
> this explanation I have just given, but it is a technicality and it 
> won't really make a difference to the user.

Yes
I would like to make it clear in this doc that DTS is compiling DPDK.
Please could you change to something like
"DTS needs a DPDK tarball or a git ref ID to compile" ?

I hope we will change it later to allow external compilation.


> >> +   (dts-py3.10) $ ./main.py --help
> > 
> > Why adding this line?
> 
> Just running `./main.py` will just throw a confusing error to the user. 
> I am in the process of sorting this out as it is misleading and not 
> helpful. Specifying the line in this case just hints to the user on the 
> origin of that help/usage document.

Yes would be good to have a message to help the user instead of a confusing error.

> > Should we remove the shell prefix referring to a specific Python version?
> 
> I have purposely left the prefix to indicate that we are in a Poetry 
> shell environment, as that is a pre-requisite to run DTS. So more of an 
> implicit reminder. The Python version specified is in line with the 
> minimum requirement of DTS.

OK

> > In general it is better to avoid long lines, and split after a punctation.
> > I think we should take the habit to always go to the next line after the end of a sentence.
> 
> I left the output of `--help` under a code block as it is originally 
> printed in the console. Could surely amend it in the docs to be easier 
> to read, but the user could as easily print it themselves in their own 
> terminal in the comfort of their own environment.

I was not referring to the console output.
Maybe I misunderstood it.
For the doc sentences, please try to split sentences on different lines.


> >> -                           [DTS_OUTPUT_DIR] Output directory where dts logs and results are
> >> -                           saved. (default: output)
> >> +                           [DTS_OUTPUT_DIR] Output directory where dts logs and results are saved.
> > 
> > dts -> DTS
> 
> As above. The output of `--help` only changed as a result of not being 
> updated before in parallel with code changes. Consistently this is what 
> the user would see right now. It may or may not be a good idea to update 
> this whenever changed in the future.

I did not understand it is part of the help message.

> Nonetheless, I am keen to update the code as part of this patch to 
> resolve your comments.

Yes please update the code for this small wording fix.

> > Please don't add compilation configuration for now,
> > I would like to work on the schema first.
> > This is mostly imported from the old DTS and needs to be rethink.
> 
> While I understand the concern on wanting to rework the schema, which is 
> a great point you make, it may be reasonable to provide something useful 
> to close the existing documentation gap. And incrementally updating from 
> there. If there is no realistic timeline set in place for a schema 
> rework, it may just be better to have something rather than nothing. And 
> certainly it would not be very useful to upstream a partial documentation.

I don't know. I have big doubts about the current schema.
I will review it with your doc patches.
Can you please split this patch in 2 so that the schema doc is in a different patch?

> Thank you a lot for your review! You have made some good points which 
> open up new potential tasks to add to the pipeline.