openzim / nautilus

Turns a collection of documents into a browsable ZIM file
GNU General Public License v3.0
19 stars 14 forks source link

Added Mandatory check for `Description`, made `longDescription` configurable and added max sizes check for both #48

Closed ankur12-1610 closed 1 year ago

ankur12-1610 commented 1 year ago

Fixes: #46

Proof of Work:

Description too long Screenshot from 2023-03-09 15-08-50

#

Description not given (i.e null or NoneType) Screenshot from 2023-03-09 15-09-06

#

LongDescription not given (the check still passes as it is optional to have LongDescription) Screenshot from 2023-03-09 15-09-30

#

LongDescription has more than 4000 chars Screenshot from 2023-03-09 15-15-44

ankur12-1610 commented 1 year ago

Screenshot from 2023-03-09 14-29-35

zimscraperlib's filesytem doesn't have anything such as LongDescription so I am unable to store it in zim_info, can you please verify if I have implemented LongDescription correctly?

ankur12-1610 commented 1 year ago

Thank you but it's not appropriate:

  • don't change formatting in your PR. Creates noise that makes review difficult and it brings inconsistency in the code base
  • long_description is not handled.
  • description's requirement should be handled by argparse.

For code formatting I used black as mentioned in docs, it did all the formatting.

Also, can you help me with how the long_description can be handled?

For description's requirement, yeah I'll use argparse :+1:

rgaudin commented 1 year ago

Screenshot from 2023-03-09 14-29-35

zimscraperlib's filesytem doesn't have anything such as LongDescription so I am unable to store it in zim_info, can you please verify if I have implemented LongDescription correctly?

Indeed it doesn't suport LongDescription and doesn't accept arbitrary metadata neither. Open a ticket on scraperlib regarding this please.

When referencing code, link to github (using the permalink feature) specifying a range of line numbers. This way, we have the code in a widget and can check it with more ease than a screenshot

rgaudin commented 1 year ago

For code formatting I used black as mentioned in docs, it did all the formatting.

black's formatting changes over versions. Find the version used in the repo

For description's requirement, yeah I'll use argparse 👍

check entrypoint.py

ankur12-1610 commented 1 year ago

For code formatting I used black as mentioned in docs, it did all the formatting.

black's formatting changes over versions. Find the version used in the repo

Will reformat

For description's requirement, yeah I'll use argparse +1

check entrypoint.py

On it!

ankur12-1610 commented 1 year ago

Screenshot from 2023-03-09 14-29-35 zimscraperlib's filesytem doesn't have anything such as LongDescription so I am unable to store it in zim_info, can you please verify if I have implemented LongDescription correctly?

Indeed it doesn't suport LongDescription and doesn't accept arbitrary metadata neither. Open a ticket on scraperlib regarding this please.

When referencing code, link to github (using the permalink feature) specifying a range of line numbers. This way, we have the code in a widget and can check it with more ease than a screenshot

If currently, any support isn't present, how will I implement long_description?

ankur12-1610 commented 1 year ago

In the latest commit, I've updated argparse as told, I've written function check_metadata because in https://wiki.openzim.org/wiki/Metadata there is Title as well who has max char limit so if future we might need to add new checks which can be appended in the implemented function.

Regarding LongDescription, I removed the changes for now, will do it once I get a clear picture of how to handle it :)

kelson42 commented 1 year ago

Any news here?

ankur12-1610 commented 1 year ago

Any news here?

Hey, the LongDescription part remains as there isn't any support available, but the description part is done.

rgaudin commented 1 year ago

If it's done, you should request a review. If there's stuff that were expected and are not present, that should be explained in the PR discussion as well.

ankur12-1610 commented 1 year ago

If it's done, you should request a review. If there's stuff that were expected and are not present, that should be explained in the PR discussion as well.

I'm really sorry I got caught up and didn't look into the GitHub Activity.

In the latest commit, I've updated argparse as told, I've written function check_metadata because in https://wiki.openzim.org/wiki/Metadata there is Title as well who has max char limit so if future we might need to add new checks which can be appended in the implemented function. Regarding LongDescription, I removed the changes for now, will do it once I get a clear picture of how to handle it :)

@rgaudin this was the follow-up description but it was left unanswered and the follow-up was given on the same day the PR was opened but I didn't receive any reply. This falls under the category of providing a valid PR description.

rgaudin commented 1 year ago

I've mentioned in one comment that for LongDescription you should open a ticket on scraperlib since it was not supported. You didn't. In the mean time, the feature has been implemented there but it's not released yet but it's on main. It's been 2 weeks and this is an easy ticket so other GSoC candidates wants to git it a shot. Haven't received another PR yet though