search

openzim / zim-tools

Various ZIM command line tools
https://download.openzim.org/release/zim-tools/
GNU General Public License v3.0
123 stars 34 forks source link

Added checks for description and longDescription length, description being non-empty #333

Closed Onyx2406 closed 1 year ago

Onyx2406 commented 1 year ago

fixes #332 and #336

kelson42 commented 1 year ago

@Onyx2406 In case this is not clear, you don't have implemented everything requested.

Onyx2406 commented 1 year ago

can you review this now? @kelson42

I made some commits

Onyx2406 commented 1 year ago

Done @kelson42

Onyx2406 commented 1 year ago

Done @kelson42 , can you review?

kelson42 commented 1 year ago

@Onyx2406 I have already approved, @veloman-yunkan is your reviewer now. You don't need to put a comment, just click on the review rerequesting icon.

Onyx2406 commented 1 year ago

Code is good. However the commit message body contains some text that has little to no value. Once the commit message is fixed this PR can be merged.

Note that std::string::length() returns the size of the string in bytes rather than in characters, i.e. the checks will not work correctly on non-ascii texts. But that can be fixed in s separate PR.

Hi, which commit message body? Can you maybe give reference.

veloman-yunkan commented 1 year ago

Hi, which commit message body? Can you maybe give reference.

Your PR contains only one commit. Its description (commit message) reads

Added checks for description and longDescription length.

Update zimwriterfs.cpp

Update zimwriterfs.cpp

Update zimwriterfs.cpp

Update zimwriterfs.cpp

added a check on description being non-empty

The first line (the commit title) is ~fine~ (I just figured out that the fact that a new option --longDescription was added is not reflected). The rest (body) doesn't add any valuable information in excess of what is found in the title.

A better commit description should be, for example:

zimwriterfs: added new option --longDescription

Also

  • made the --description option mandatory (its value cannot be empty)
  • checking that the lengths of the short and long descriptions are below their respective limits (80 and 4000)
  • the long description is optional; however, if provided, it must not be shorter than the short description
Onyx2406 commented 1 year ago

The title of the commit message is too long - it exceeds the recommended limit, hence part of the title is moved into the body. If you want to mention the new checks in the title, you better split your changes into two commits:

  1. Introduce the checks on --description option
  2. Add --longDescription option (along with its own checks, but the latter part doesn't need to be in the commit title)

Looks good now..

kelson42 commented 1 year ago

@veloman-yunkan @Onyx2406 Part of the CI does not pass, it seems to me we have a regression here.

veloman-yunkan commented 1 year ago

@veloman-yunkan @Onyx2406 Part of the CI does not pass, it seems to me we have a regression here.

Indeed. I missed a missing closing brace during review since I trust the compiler. Looks like @Onyx2406 didn't build the latest code before requesting review.

veloman-yunkan commented 1 year ago

@kelson42 BTW, I wonder why the list of CI jobs in this PR is limited to Packages workflows.

mgautierfr commented 1 year ago

@kelson42 BTW, I wonder why the list of CI jobs in this PR is limited to Packages workflows.

This is because the PR is coming from a branch in another repository. And the CI workflow is not run for security reasons. However, we may have to check the exact status about that and do something to homogenize all CIs.

Onyx2406 commented 1 year ago

@veloman-yunkan @Onyx2406 Part of the CI does not pass, it seems to me we have a regression here.

Indeed. I missed a missing closing brace during review since I trust the compiler. Looks like @Onyx2406 didn't build the latest code before requesting review.

should pass now @veloman-yunkan