sstarcher / paprika-exporter

Export recipe data out of Paprika into YAML
Apache License 2.0
12 stars 4 forks source link

Adds full photo fetch and status check #3

Closed datapolitical closed 3 years ago

datapolitical commented 3 years ago

This pull request adds the ability to get all of the available photos of a recipe, it also runs a status check so that if nothing has changed, the script completes without re-downloading everything.

it also changes the directory structure of the project in order to comply with the requirements of python packaging. I wanted to reuse the code across multiple repos and this was the easiest way to do so.

sstarcher commented 3 years ago

Conflicts to resolve in export.py and READM.me and I can review it later this week.

sstarcher commented 3 years ago

The pipfile should be updated to add the things that are in the requirements.txt. I would recommend removing the requirements.txt itself and if needed using pipfile to generate it on the fly.

sstarcher commented 3 years ago
❯ ~/github/paprika-exporter/papexp/export.py
Traceback (most recent call last):
  File "/Users/shanestarcher/github/paprika-exporter/papexp/export.py", line 130, in <module>
    check_and_run()
  File "/Users/shanestarcher/github/paprika-exporter/papexp/export.py", line 24, in check_and_run
    with open(r'./_data/recipes_status.json', 'rb') as file:
FileNotFoundError: [Errno 2] No such file or directory: './_data/recipes_status.json'
sstarcher commented 3 years ago

I manually created the _data/recipes_status.json after doing so I got the following error

Traceback (most recent call last):
  File "/Users/shanestarcher/github/paprika-exporter/papexp/export.py", line 130, in <module>
    check_and_run()
  File "/Users/shanestarcher/github/paprika-exporter/papexp/export.py", line 32, in check_and_run
    export_recipes()
  File "/Users/shanestarcher/github/paprika-exporter/papexp/export.py", line 69, in export_recipes
    local_file = open('assets/images/recipes/'+recipe['photo'], 'wb')
FileNotFoundError: [Errno 2] No such file or directory: 'assets/images/recipes/xxxxxxxxx.jpg'
sstarcher commented 3 years ago

After making the above changes by creating the two folders I now get no output from ❯ ~/github/paprika-exporter/papexp/export.py

sstarcher commented 3 years ago

After deleting the _data/recipes_status.json it downloaded the files. From my quick glance it looks like if the script runs and fails mid way through due to a bug or a connection issue the status file will block any updates even though the script failed. One solution would be to catch any errors and cleanup the status file if the script fails.

sstarcher commented 3 years ago

Ah, thanks I did not realize it was actually in pypy. Now that it has multiple files we should probably call out export.py or move export.py to the root level as it's no longer immediately obvious that you can call it as a python script

On Tue, Oct 12, 2021 at 10:06 AM Chris Nicholson @.***> wrote:

@.**** commented on this pull request.

In README.md https://github.com/sstarcher/paprika-exporter/pull/3#discussion_r727233541 :

Export Paprika data using the API to yaml

-# Why

+# Introduction

+

+This tool statically generates a list of Paprika recipes in YAML format, with a related folder of images.

+

+# Installation

+

+pip3 install papexp

Clarifying this makes sense. The repository exists as a python package on pypi so it runs without needing to clone (because when I first updated it that was the only way I could figure out how to run it from another repository's GitHub action).

But it's obviously not strictly necessary to use that version.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sstarcher/paprika-exporter/pull/3#discussion_r727233541, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA42THXS2GUVQCCQCZUOY2LUGRFHRANCNFSM5FIWVE5Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

datapolitical commented 3 years ago

That makes perfect sense.

datapolitical commented 3 years ago

This is gonna sound dumb, but how do I do that without breaking the package? For the life of me I couldn't figure out how to make it package while keeping the script in the root directory like you had it. Which is why the structure changed.

datapolitical commented 3 years ago

OK, I've made all the changes. Let me know how it looks

sstarcher commented 3 years ago

I would separate the library code from the export.py code. And I would create a file in the root that imports papexp and executes whatever code is needed

sstarcher commented 3 years ago

Please add a bash shebang for python to the top of export.py and add write permissions to the file. You will also need to rebase on master as you still have conflicts.

sstarcher commented 3 years ago

This looks to still be broken

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/shanestarcher/github/paprika-exporter/export.py", line 9, in <module>
    papexp.core.check_and_run()
  File "/Users/shanestarcher/github/paprika-exporter/papexp/core.py", line 28, in check_and_run
    with open(r'./_data/recipes_status.json', 'wb') as file:
FileNotFoundError: [Errno 2] No such file or directory: './_data/recipes_status.json'
mv: temp/recipes.yaml: No such file or directory
datapolitical commented 3 years ago

That's weird, it didn't show up when I was testing it. Should be fixed now.

What's the right way to re-base in this situation? I haven't used the command before, and the documentation on it is pretty scary and confusing.

sstarcher commented 3 years ago

It depends on how your remotes are setup. Let's you you cloned github.com/sstarcher/paprika-exporter git remote -v would show your remotes.

You would run git rebase origin/master <- origin should be what shows up when you run git remote -v and says sstarcher if it does not say origin you need to use whatever it says.

datapolitical commented 3 years ago

Got it. That would be upstream.

And this would basically reset the head to the newest commit on your branch.

I think I understand how to squash the commits, so I'll try to do that to make the branch a little cleaner since I have so many random ones.

datapolitical commented 3 years ago

Okay I've rebased and added the shebang. I'm not sure what you meant about adding write permissions on export.py

sstarcher commented 3 years ago

@datapolitical Can you attempt to run export.py from a command line? If you do it will fail due to not having the correct permissions. I'm going to guess you are on a Windows machine?

datapolitical commented 3 years ago

No I'm on a Mac but I use

python export.py

I've updated the file permissions but now the command line arguments aren't working. Any thoughts on how to make it work for both ./export.py and python export.py?

sstarcher commented 3 years ago

export.py must have #!/usr/bin/env python3 at the top for ./export.py to work. The current code does not have that and I also see your conflicts are not resolved.

datapolitical commented 3 years ago

Sorry about the difficulties there, it took me a bit to figure out what you meant, this is my first time doing a pull request with conflicts.

datapolitical commented 3 years ago

All the conflicts have been resolved. (He says with tenuous confidence) everything should be good to go now.

sstarcher commented 3 years ago

@datapolitical excellent thanks for the work. I'll review it in a bit

sstarcher commented 3 years ago

I'm good with the existing changes let me know if you are good with me squashing and merging it.

datapolitical commented 3 years ago

Yep, it's good to go. 

sstarcher commented 3 years ago

@datapolitical thanks for all of the hard work and working through my comments

datapolitical commented 3 years ago

Totally. I can also add you as an owner for the pypi package so you can push new releases going forward, since your repository is the master.

What's your pypi username?

sstarcher commented 3 years ago

Thanks it's sstarcher

datapolitical commented 3 years ago

It's saying you need to verify your primary email address with them before you can be added.

sstarcher commented 3 years ago

done

sstarcher commented 3 years ago

done

On Thu, Oct 14, 2021 at 9:07 AM Chris Nicholson @.***> wrote:

It's saying you need to verify your primary email address with them before you can be added.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/sstarcher/paprika-exporter/pull/3#issuecomment-943393285, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA42THWLFLAFC6YZMZMSCU3UG3P23ANCNFSM5FIWVE5Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

datapolitical commented 3 years ago

Sent. I'll let you decide how to handle versioning going forward. I had this update, minus the changes in the last 24 hours, as version 0.4.1. Once we've tested it fully we could bump that to 0.5.0.