ocaml
Document ocamltest builtin variables and actions
Although they can be found in the source, it seems to me a saner approach to document them in the manual page.
Although they can be found in the source, it seems to me a saner approach to document them in the manual page.
They can also be found by doing ocamltest -show-variables.
In that case, maybe this useful flag can be also mentioned in the document. (We could make it the only change proposed by this PR, but I personally think that duplicating information in several relevant places decreases the mean time people will spend searching for it.)
I added the list of builtin actions. This is even more useful as -show-actions does not output a description of each action, and one has to refer to the source. Even there, some actions are non obvious to figure out.
Most of this is a dirty copy-paste from the descriptions in ocamltest source, but someone should check that I didn't make a mistake in documenting the few that had no such description.
I also added the actions set and unset which exist, but are not present in the output of -show-actions.
Are you aware of ocamltest's -show-variables commandline option?
Not to say it would not be useful to document the variables also in the manual, but rather so that you can take this fact into account to decide whether you still would like to submit this PR and, if so, whether knowing this makes you want to change its contents.
Nicolás Ojeda Bär (2022/08/26 07:18 -0700):
Although they can be found in the source, it seems to me a saner approach to document them in the manual page.
They can also be found by doing
ocamltest -show-variables.
Oops, thanks @nojb and sorry I missed this message.
Olivier Nicole (2022/08/26 07:33 -0700):
In that case, maybe this useful flag can be also mentioned in the document. (We could make it the only change proposed by this PR, but I personally think that duplicating information in several relevant places decreases the mean time people will spend searching for it.)
Perhaps, but it increases the maintainance burden and creates the risk that the different copies get de-synchronized.
Another option would be to have some parts of the manual generated.
Olivier Nicole (2022/08/30 01:24 -0700):
@shindere This discussion started in the messages preceding yours.
Yes, seen now. Thanks. Sorry for the noise.
So the possible solutions seem to be:
- Generate parts of the manual.
- In the manual, only mention the existence of the
-show-variablesand-show-actionsflags.
In both cases, we would benefit from fixing the fact that ocamltest -show-actions does not show descriptions, and that set and unset are not listed.
Very sorry for the late feedback on this.
Olivier Nicole (2022/08/26 09:13 -0700):
I added the list of builtin actions.
Thanks a lot!
This is even more useful as
-show-actionsdoes not output a description of each action, and one has to refer to the source. Even there, some actions are non obvious to figure out.
I am sorry about having created things that way. Apologies. I think initially actions were not intended to be that visible.
I'd like to suggest adding an action_description field to the
Actions.t type where the description could be stored ahdn then used
int he help output, and similar for tests. What do you think?
Most of this is a dirty copy-paste from the descriptions in ocamltest source, but someone should check that I didn't make a mistake in documenting the few that had no such description.
I'll try to check at some point but let's first clarify if adding the fields is okay. I can also do it if you like, e.g. in another PR, and integrate / proof read the documentation you gathered (many thanks for that).
I also added the actions
setandunsetwhich exist, but are not present in the output of-show-actions.
Many thanks for giving me the opportunity to think about it. In fact, I
had never seen set and unset as actions. I always considered them as
being part of the DSL but being distinct from actions.
Perhaps it would make sense to consider them as actions, I dont really know. The thing is, if they are considered actions, then it feels to me the quesiton becomes wider and extends to all operations on variables (assigning, including environments...). Should everything become an action, actually?
One side-effect would be that every assignment, set, unset etc., would
give rise to a displayed line in the output of ocamltest.
Olivier Nicole (2022/08/30 05:03 -0700):
So the possible solutions seem to be:
- Generate parts of the manual.
- In the manual, only mention the existence of the
-show-variablesand-show-actionsflags.In both cases, we would benefit from fixing the fact that
ocamltest -show-actionsdoes not show descriptions, and thatsetandunsetare not listed.
I think my previous comment addressed all these?
I'd like to suggest adding an
action_descriptionfield to theActions.ttype where the description could be stored ahdn then used int he help output, and similar for tests. What do you think?
Sounds good to me.
Many thanks for giving me the opportunity to think about it. In fact, I had never seen
setandunsetas actions. I always considered them as being part of the DSL but being distinct from actions. Perhaps it would make sense to consider them as actions, I dont really know. The thing is, if they are considered actions, then it feels to me the quesiton becomes wider and extends to all operations on variables (assigning, including environments...). Should everything become an action, actually? One side-effect would be that every assignment, set, unset etc., would give rise to a displayed line in the output ofocamltest.
Hmm, maybe it makes little sense for them to be actions then. (I sort of implicitly assumed that everything that is not prefixed by one or more stars is an action or a variable assignment, but I see that things are more subtle.) In that case we don't necessarily need to make set/unset an action, however I think they are worth mentioning in the manual, for people who don't already know about them.
@shindere As discussed, I have added a description field to the Actions.t type, which allows -show-actions to output a brief description of each action. I have updated the manual page correspondingly, and added a mention of the set and unset keywords.
Let me know if you are OK with the way it is done. The descriptions could also probably use proofreading.
Many thanks for the great work, @OlivierNicole.
Globally everything looks very good to me.
In the OCaml test manual there is a repetition of the word "variables".
Also, as far as I remember, the set primitive adds a variable to
ocamltest's environment and it then gets exported to the system
environment. The same is true for unset, it affects ocamltest's
environemnt rather than the system environment, I think.
Do you intend to also add a descriptin field to the test type?
In the descriptions themselves, youmay want to replace 32-bit by
32-bits and likewise for 64-bit.
Finally, some descriptions use third person, others use an infinitive form. That's something you may want to make coherent. My personal preference would be for all-infinitive.
All that being said: if you are fed up with this PR Iwoul'nt mind merging it as-is, because it does improve the situation already in its current state, and we can always open a second one to continue. Up to you.
I can do it, no problem, I’ll just need to finish something else first so it would be later this week. Edit: And thanks for the review!
Olivier Nicole (2022/10/03 08:05 -0700):
I can do it, no problem, I’ll just need to finish something else first so it would be later this week.
No pressure!
Regarding the names of the fields, I think in variables.ml the
description field is called variable_description so one could use
action_description and test_description to be coherent.
Update: during a live discussion, @oliviernicole and I agreed that
Olivier will add the action_description and test_description fields
where appropriate and fill them as best as he can and I willreview and
propose descripitons for the missing items.
Apologies for taking such a long time to come back to this PR. I have added three more commits to address two of you remarks, namely, to fix the typo you noted and to switch action descriptions to infinitive, and to clarify the effect of set and unset (I removed the notion of “system” environment variables to only talk about environment variables for a given test, which I think clearer). We can squash all or some of these commits if needed.
Below are questions and comments on the rest of your remarks:
Do you intend to also add a descriptin field to the test type?
I realize now that I am not sure which type you mean. Do you mean the type t from the Tests module? If so, I am not sure what adding a description field would change for the user of ocamltest.
In the descriptions themselves, youmay want to replace
32-bitby32-bitsand likewise for64-bit.
I believe that the rule in English is that when “32 bits” is in adjectival position, you have to remove the S. See e.g. the Wikipedia page for 32-bit computing.
Many thanks for your work on this!
Apologies for taking such a long time to come back to this PR. I have added three more commits to address two of you remarks, namely, to fix the typo you noted and to switch action descriptions to infinitive, and to clarify the effect of
setandunset(I removed the notion of “system” environment variables to only talk about environment variables for a given test, which I think clearer). We can squash all or some of these commits if needed.
Yeah I think it will be good to squash all the commits into one.
Below are questions and comments on the rest of your remarks:
Do you intend to also add a descriptin field to the test type?
I realize now that I am not sure which type you mean. Do you mean the type
tfrom theTestsmodule? If so, I am not sure what adding a description field would change for the user of ocamltest.
Meanwhile we clarified this during an off-line discussion.
In the descriptions themselves, youmay want to replace
32-bitby32-bitsand likewise for64-bit.I believe that the rule in English is that when “32 bits” is in adjectival position, you have to remove the S. See e.g. the Wikipedia page for 32-bit computing.
Oh I was not aware, sorry and thanks!
Sorry for the delay. As discussed, I added a test_description field to the Tests.t record, which is now printed by the -show-tests option. I added a brief description of each test in ocaml_tests.ml. I also renamed the description field in Actions.t to action_description.
I just pushed a mention of -show-tests in the manual. Sorry for the noise.
I believe that I have addressed all of your requests so far.
Thansk a lot, Olivier! And, no problem.
Are there other things you'd like to do on this or do you consider it to be ready to be final-reviewed /merged?
Final review done.
I have pushed two suggestions to the pr-11514-patches branch of
shindere/ocaml, in two commits so that you can decide for each of them
whether you want it or not. Let me know if you like them and if that's
the case I suggest you sqhash whatever you want to take in your commit
and then I can merge nomatter whether you take 0, 1 or 2 of them.
Both commits made perfect sense to me so I squashed them both in, thank you.
Olivier Nicole (2023/01/17 04:49 -0800):
Both commits made perfect sense to me so I squashed them both in, thank you.
Thanks to you for being so open and of such a good will!
Let's wait until CI is happy again and then I'll merge.