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

Link and Path fields added #109

Merged
merged 8 commits into from Apr 6, 2022
Merged

Link and Path fields added #109

merged 8 commits into from Apr 6, 2022

Conversation

sadrasabouri
Copy link
Collaborator

Reference Issues/PRs

#99

What does this implement/fix? Explain your changes.

Since we need to know what's the link of uploaded art in the nft storage we decided to return the link to the art in the when using nft_storage_upload method.
Same issue when saving a config/data/art file.

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #109 (9758589) into dev (4f25f84) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #109      +/-   ##
==========================================
+ Coverage   99.47%   99.48%   +0.01%     
==========================================
  Files           4        4              
  Lines         372      378       +6     
  Branches       58       58              
==========================================
+ Hits          370      376       +6     
  Partials        2        2              
Impacted Files Coverage Δ
samila/functions.py 99.22% <100.00%> (+0.02%) ⬆️
samila/params.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f25f84...9758589. Read the comment docs.

Copy link
Owner

@sepandhaghighi sepandhaghighi left a comment

Choose a reason for hiding this comment

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

@sadrasabouri Thanks for your efforts 💯
JSON field shouldn't be dynamic (sometimes message sometimes link/path)
I suggest replacing link and path with message

@sadrasabouri
Copy link
Collaborator Author

sadrasabouri commented Apr 5, 2022

I suggest replacing link and path with message

I think message field is necessary in some cases in which we return error message in message field and these error messages can not be in the link/path field.

samila/params.py Outdated
@@ -28,6 +28,7 @@
SEED_UPPER_BOUND = 2**20
VALID_COLORS = list(dict(mcolors.BASE_COLORS, **mcolors.CSS4_COLORS).keys())
NFT_STORAGE_API = "https://api.nft.storage/upload"
NFT_STORAGE_LINK = "https://{}.ipfs.nftstorage.link/"
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest this format:
https://ipfs.io/ipfs/{}

@@ -339,6 +342,7 @@ def save_data_file(g, file_adr):
try:
with open(file_adr, 'w') as fp:
json.dump(data, fp)
result["message"] = os.path.dirname(os.path.abspath(file_adr))
Copy link
Owner

Choose a reason for hiding this comment

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

I think a direct file address (os.path.abspath(file_adr)) is better than a folder address. isn't?

@sepandhaghighi sepandhaghighi added this to the samila v0.6 milestone Apr 6, 2022
@sepandhaghighi sepandhaghighi added enhancement New feature or request new feature labels Apr 6, 2022
Copy link
Owner

@sepandhaghighi sepandhaghighi left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@sepandhaghighi sepandhaghighi merged commit 24cec90 into dev Apr 6, 2022
@sepandhaghighi sepandhaghighi deleted the link-path branch April 6, 2022 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants