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

Onyx2406 · 2023-03-08T09:49:19Z

fixes #332 and #336

kelson42 · 2023-03-08T16:15:36Z

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

Onyx2406 · 2023-03-18T17:39:38Z

can you review this now? @kelson42

I made some commits

kelson42

Update the usage() as well

src/zimwriterfs/zimwriterfs.cpp

Onyx2406 · 2023-03-19T15:15:32Z

Done @kelson42

kelson42

LGTM, requesting @veloman-yunkan for a more technical review.

veloman-yunkan

Please squash all commits into one.

src/zimwriterfs/zimwriterfs.cpp

veloman-yunkan · 2023-03-20T13:18:13Z

src/zimwriterfs/zimwriterfs.cpp

+ if (description.length() > 80) {
+ std::cerr << "Description length exceeds the 80 character limit." << std::endl;
+ exit(1);
+ }


Please fix the indentation

Do we allow empty descriptions? Should we impose a lower limit on the length of the short description?

Do we allow empty descriptions?

#336 says "no". So we can fix that ticket in this PR too.

src/zimwriterfs/zimwriterfs.cpp

Onyx2406 · 2023-03-21T06:21:22Z

Done @kelson42 , can you review?

kelson42 · 2023-03-21T06:29:18Z

@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.

veloman-yunkan

The commit description must be edited

src/zimwriterfs/zimwriterfs.cpp

veloman-yunkan

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.

Onyx2406 · 2023-03-21T17:18:46Z

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 · 2023-03-21T18:53:59Z

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

veloman-yunkan

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:

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

Onyx2406 · 2023-03-22T09:38:05Z

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:

Introduce the checks on --description option

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 · 2023-03-22T09:56:15Z

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

src/zimwriterfs/zimwriterfs.cpp

veloman-yunkan · 2023-03-22T10:26:31Z

@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 · 2023-03-22T10:28:36Z

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

mgautierfr · 2023-03-22T10:50:40Z

@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 · 2023-03-22T13:51:24Z

@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

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

kelson42 marked this pull request as draft March 8, 2023 09:51

Onyx2406 marked this pull request as ready for review March 19, 2023 10:01

rgaudin requested a review from kelson42 March 19, 2023 11:29

kelson42 requested changes Mar 19, 2023

View reviewed changes

src/zimwriterfs/zimwriterfs.cpp Outdated Show resolved Hide resolved

src/zimwriterfs/zimwriterfs.cpp Outdated Show resolved Hide resolved

kelson42 requested review from kelson42 and veloman-yunkan March 19, 2023 15:18

kelson42 approved these changes Mar 19, 2023

View reviewed changes

kelson42 force-pushed the main branch from 5b196d7 to 5878bfe Compare March 19, 2023 15:23

veloman-yunkan requested changes Mar 20, 2023

View reviewed changes

Onyx2406 force-pushed the main branch 3 times, most recently from a305a39 to c922868 Compare March 21, 2023 06:20

Onyx2406 requested a review from veloman-yunkan March 21, 2023 06:49

veloman-yunkan requested changes Mar 21, 2023

View reviewed changes

src/zimwriterfs/zimwriterfs.cpp Outdated Show resolved Hide resolved

src/zimwriterfs/zimwriterfs.cpp Outdated Show resolved Hide resolved

Onyx2406 force-pushed the main branch from efce1e8 to a798928 Compare March 21, 2023 16:46

Onyx2406 changed the title ~~Added checks for description and longDescription length.~~ Added checks for description and longDescription length, description being non-empty Mar 21, 2023

Onyx2406 requested a review from veloman-yunkan March 21, 2023 16:49

veloman-yunkan requested changes Mar 21, 2023

View reviewed changes

Onyx2406 force-pushed the main branch 2 times, most recently from 80b5658 to 7333904 Compare March 21, 2023 19:26

Onyx2406 requested a review from veloman-yunkan March 21, 2023 19:27

veloman-yunkan reviewed Mar 21, 2023

View reviewed changes

Onyx2406 force-pushed the main branch from 7333904 to e295f9f Compare March 22, 2023 09:34

Onyx2406 force-pushed the main branch from e295f9f to a740c6e Compare March 22, 2023 09:36

Onyx2406 requested a review from veloman-yunkan March 22, 2023 09:37

veloman-yunkan approved these changes Mar 22, 2023

View reviewed changes

kelson42 force-pushed the main branch from a740c6e to 4da328b Compare March 22, 2023 09:51

veloman-yunkan reviewed Mar 22, 2023

View reviewed changes

src/zimwriterfs/zimwriterfs.cpp Outdated Show resolved Hide resolved

Onyx2406 force-pushed the main branch 2 times, most recently from f8a7f8a to ec80394 Compare March 22, 2023 13:28

kelson42 force-pushed the main branch from ec80394 to c14bc96 Compare March 22, 2023 14:00

kelson42 merged commit 14c7277 into openzim:main Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Onyx2406 commented Mar 8, 2023 •

edited

Loading

kelson42 commented Mar 8, 2023

Onyx2406 commented Mar 18, 2023

kelson42 left a comment

Onyx2406 commented Mar 19, 2023

kelson42 left a comment

veloman-yunkan left a comment

veloman-yunkan Mar 20, 2023

veloman-yunkan Mar 20, 2023

Onyx2406 commented Mar 21, 2023

kelson42 commented Mar 21, 2023

veloman-yunkan left a comment

veloman-yunkan left a comment

Onyx2406 commented Mar 21, 2023

veloman-yunkan commented Mar 21, 2023

veloman-yunkan left a comment

Onyx2406 commented Mar 22, 2023

kelson42 commented Mar 22, 2023

veloman-yunkan commented Mar 22, 2023

veloman-yunkan commented Mar 22, 2023

mgautierfr commented Mar 22, 2023

Onyx2406 commented Mar 22, 2023

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

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

Conversation

Onyx2406 commented Mar 8, 2023 • edited Loading

kelson42 commented Mar 8, 2023

Onyx2406 commented Mar 18, 2023

kelson42 left a comment

Choose a reason for hiding this comment

Onyx2406 commented Mar 19, 2023

kelson42 left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Mar 20, 2023

Choose a reason for hiding this comment

veloman-yunkan Mar 20, 2023

Choose a reason for hiding this comment

Onyx2406 commented Mar 21, 2023

kelson42 commented Mar 21, 2023

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

Onyx2406 commented Mar 21, 2023

veloman-yunkan commented Mar 21, 2023

veloman-yunkan left a comment

Choose a reason for hiding this comment

Onyx2406 commented Mar 22, 2023

kelson42 commented Mar 22, 2023

veloman-yunkan commented Mar 22, 2023

veloman-yunkan commented Mar 22, 2023

mgautierfr commented Mar 22, 2023

Onyx2406 commented Mar 22, 2023

Onyx2406 commented Mar 8, 2023 •

edited

Loading