search

openzim / nautilus

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

Mandatory Description + LongDescription introduction #53

Closed KrishnaRishi2208 closed 1 year ago

KrishnaRishi2208 commented 1 year ago

I have added description as mandatory and checked if the description provided is below 80 characters. I have also added the option to provide longdescription which has a maximum length of 4000 characters. For users to see the longdescription, I have changed the about_content.

I checked if the user has given a file as input or a text as input for both description, longdescription .

I have checked if longdescription was provided before checking the length of the long description.

I have made the following changes in zimscraperlib/zim/filesystem.py for the above code to work:

1)After line 119("description: str,") I added: long_description:str, 2)After line 162(""description": description,") I added: "long_description":long_description,

These were the changes I made.

rgaudin commented 1 year ago

Also this is a poorly chosen name for a PR

rgaudin commented 1 year ago

And PR must be linked to the related issue in Github UI (I did it)

rgaudin commented 1 year ago

Your commits should also have meaningful names. Once the PR is accepted, you'll have to fix it. For such a fix, a single commit will be enough.

KrishnaRishi2208 commented 1 year ago

Also this is a poorly chosen name for a PR

Since this is my first time trying to solve an issue, I had some trouble naming it. Thanks for letting me know. I'll change the name of the PR next time.

KrishnaRishi2208 commented 1 year ago

Thank you ; please also update the requirement file to use zimscraperlib 3.0.0 and verify it's working as expected

I have checked zimscraperlib 3.0.0 . I have been using 1.1.1 version until now. In the latest versions, the imaging.py file is not present in the zimscraperlib which is used for resizing image, checking hex colors, creating favicons , getting colors. The bytecode is present in the pycache of zimscraperlib, but the file is missing from the distribution. So , I will create the imaging.py file locally in the nautilus repository as it is important. Is that ok?

rgaudin commented 1 year ago

So , I will create the imaging.py file locally in the nautilus repository as it is important. Is that ok?

No. scraperlib's API changed obviously. I didn't realize nautilus was using such an old version.

Do you want to try a different PR that simply updates scraperlib? You just need to change the requirements and update all the API that changed. Once this gets merged we'll be able to merge the current PR. If that sounds too difficult for you, let me know and I'll do it

KrishnaRishi2208 commented 1 year ago

So , I will create the imaging.py file locally in the nautilus repository as it is important. Is that ok?

No. scraperlib's API changed obviously. I didn't realize nautilus was using such an old version.

Do you want to try a different PR that simply updates scraperlib? You just need to change the requirements and update all the API that changed. Once this gets merged we'll be able to merge the current PR. If that sounds too difficult for you, let me know and I'll do it

Sure, I'll create a PR in scraperlib where I will add imaging.py and update the requirements. But, if scraperlib has removed imaging.py , it means that the functionality is not required in scraperlib anymore right. So, isnt it better to create imaging.py in nautilus as imaging.py does not have any functionality that requires modules from scraperlib .

KrishnaRishi2208 commented 1 year ago

@rgaudin I think I have made all the requested changes. Please review them and inform me if any other changes are needed.

rgaudin commented 1 year ago

@rgaudin I think I have made all the requested changes. Please review them and inform me if any other changes are needed.

Several comments were not addressed. Please make sure everything is done before re-requesting. I am taking time to review your work, make it worth it.

And use the github's button to request a review

KrishnaRishi2208 commented 1 year ago

I have created proper API's for scraperlib version 3.0.0 . I have updated the requirements.txt file. I have updated the init.js file. I have updated the description and long_description to have dynamic lengths which are defined in constants.py in scraperlib. I have tested the code and it is working correctly with the proper errors . I have removed unnecessary comments.

Thank you for taking your time @rgaudin to review the code. As a beginner, I have learnt a lot by working on this issue .

Please go through the changes and inform me if any other changes are required.

KrishnaRishi2208 commented 1 year ago

@rgaudin Could you please review the PR so that I can add this to my GSOC proposal.