Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

business/offering data models and helper scripts #174

Merged

Conversation

anizeani
Copy link
Contributor

@anizeani anizeani commented Feb 23, 2022

closes #171
closes #172
Instead of passing the json file path in the terminal, i open a dialog where the user selects the file.. I thought it to be more user friendly.

added business jsons
@anizeani anizeani force-pushed the implement_business_and_offering_data_models_and_helper_scripts branch from 8d6a98f to 824243b Compare February 23, 2022 13:44
@@ -0,0 +1,7 @@
{
"name": "Küche Edison",
"description": "wir sind ein lokales Geschäft im Technopark und offerieren köstlichen Kaffe",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be visible worldwide. I'd keep it informal "bei uns gibts köstlichen Kaffee". Shouldn't sound serious in any way

@@ -0,0 +1,4 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on folder naming:

please replace "technopark" with "EdisonPaula"
subfolders "businesses / offerings do not make sense for me as it isn't clear if you want to group by business and its offerings, or as you did. I suggest to reflect business and product in filenames to reflect the type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i now created a folder structure of test-data/bazaar/business/products, ok?

…t creation, expect already existing account

uploading product.json to ipfs
verifying that name and description are present in json file
add warning if logo is missing
create offering and upload to ipfs
@anizeani anizeani requested a review from brenzi February 28, 2022 09:52
Copy link
Member

@brenzi brenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please never use umlauts in file/folder names
On the other hand: I would use Umlauts in the Description because that will be a Test we want to pass

client/upload-image-to-ipfs-and-return-cid.py Outdated Show resolved Hide resolved
client/upload-image-to-ipfs-and-return-cid.py Outdated Show resolved Hide resolved
try:
image_cid = Ipfs.add(logo_path, ipfs_local)
except:
print("failed to add image to ipfs")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please exit with an error code and don't just print an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see client.py for examples how to do that

Copy link
Contributor Author

@anizeani anizeani Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen how you check the return codes and depending on them raise a specific error.
in our case we do ipfs.add, which either adds remotely or locally.
In any case we are in an elevated level, so the error would emerge from deeper in code.
So for the curl command (remote ipfs add) there are 96 error return codes. (https://everything.curl.dev/usingcurl/returns)
any of them are actually already handled below, we just need to add the "check" option where the curl command gets excecuted (see my latest file change in file py_client/ipfs.py.
I'm not sure I understand if you want me to handle those errors in the ipfs class by myself, because those errors are actually already handled by the subprocess.run function https://github.com/python/cpython/blob/main/Lib/subprocess.py#L528
what i did now is, enable the error raising in the deeper level and i extend the error output in case of raised error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just define a positive integer number which isn't used already and throw that. Don't care what the exact error was deeper down. just ensure we exit with a positice exit code

@anizeani anizeani requested a review from brenzi February 28, 2022 14:51
@brenzi
Copy link
Member

brenzi commented Feb 28, 2022

Now we have quite a mess of scripts:

  • register-business-simple (now broken because of path change)
  • register-businesses (now broken because of path change)
  • register-business
  • register-offering
  • upload-folder-to-ipfs-and-return-cid (sounds generic, but in the help comment it says Register a business on chain)
  • upload-folder-to-ipfs-and-return-cid

then we also have

  • publish-icons.py which also just uploads stuff to ipfs

And there is no piece of doc in the README about what to use when and how.

I'll need more time to review and find a solution. Please stop working on this PR for now @anizeani .

@brenzi brenzi mentioned this pull request Aug 22, 2022
Copy link
Member

@brenzi brenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaned up bits and pieces, streamlined and documented. all works locally

@brenzi brenzi merged commit 2bc5eb1 into master Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants