Closed adrienave closed 6 months ago
Hi, thanks for the PR, I'll try to review it as soon as possible, but probably not until Monday 19.
As for the points you've made:
papi-web.ini
in this PR;1
, 2
and 3
, I don't think we're ready to deprecate yet… This should be an issue;Since they're all documentation-related, I think you should open only one issue with different tasks.
Thanks again for the PR, feel free to contribute to the code if you want to do so :)
You can probably correct the style inconsistency in papi-web.ini in this PR;
I don't think I can ; I'm talking about the default configuration provided as part of the latest build version (https://github.com/papi-web-org/papi-web/releases/tag/2.1.6).
I can't find any occurrence of either papi-web.ini
or host: 0.0.0.0
in the project that would be related to the generation of this file, I believe it's some manual action done at release time.
[...] you'd see the warnings in the logs [...]
I didn't see any in the dedicated page in the web application, but maybe it is present in the terminal logs? Or maybe my logging level is wrong.
I think we could add descriptive names in addition to 1, 2 and 3, I don't think we're ready to deprecate yet… This should be an issue;
Right, how did I forget about backward compatibility...
So sure, I will open an issue for all these points!
Thanks again for the PR, feel free to contribute to the code if you want to do so :)
That's what I'm considering yes, but I didn't start investigating the code yet.
This PR addresses a few typos/errors I found while reading the docs for the first time, as I wanted to understand better how this application is working.
Please check carefully my changes because I might have overlooked some details/done some mistakes, since I didn't use this application on a real scale yet.
I also take the opportunity to list some suggestions for amelioration, don't hesitate to tell me if I should convert them to a single issue, a bunch of issues or even fix some in this PR.
papi-web.ini
content specifieshost
andport
properties with:
instead of=
, which looks like the standard according to docs.menu_text
is required for the screen to be displayed as a menu, it was misleading for me at first, sincemenu
property is introduced first, but adding only it to my screens for testing was not changing anything in the application, nor displaying any warning/error in the dedicated space. This information is specified in ref section (40) though.before
,soon
andafter
(instead of1
,2
and3
).families
property can take a list of families, I thoughtfamilies
was just a misleading name, and noticed later (in ref section 40) that it can be also a list.PS: These are just some neat pickings, it's truly amazing you built such a dense, clean and easy to follow documentation, lots of softwares should take example on this, thank you!