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

Add dump_content function to avoid unclear item_content transformation process #90

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MHHukiewitz
Copy link
Member

In order to avoid mistakes when creating the item_content of a message, the extended_json_encoder has been added from the aleph-sdk-python package to this central piece of the aleph architecture.

Based on #89

@hoh
Copy link
Member

hoh commented Feb 1, 2024

Can you explain the issue in more details ? I do not understand what the problem is from this short description.

@MHHukiewitz
Copy link
Member Author

Well, I figured that some functions in connection with messages are too important, to have them spread out around the different repos. IMHO we want:

  • The whole item_hash generation process to be contained in this repo as reference for other language implementations (like TypeScript).
  • Standardized serialization, like here in dump_content, on which also the item_hash generation process relies.

This way we can also write tests for the validators, to make sure that what transformations we implemented here, are working fine with the current message spec.

@hoh
Copy link
Member

hoh commented Feb 13, 2024

This branch is mixing unrelated stuff, can you move black and forbid to other branches ?

aleph_message/utils.py Outdated Show resolved Hide resolved
aleph_message/utils.py Outdated Show resolved Hide resolved
aleph_message/models/__init__.py Show resolved Hide resolved
aleph_message/models/__init__.py Outdated Show resolved Hide resolved
@hoh
Copy link
Member

hoh commented Feb 13, 2024

Is the code coverage increasing or at least identical after these changes ? I don't see changes in the tests.

@MHHukiewitz
Copy link
Member Author

Is the code coverage increasing or at least identical after these changes ? I don't see changes in the tests.

93% -> 92%, due to the decreasing coverage on the utils.py file from 100% -> 72%. Will increase coverage

@MHHukiewitz MHHukiewitz requested a review from hoh March 14, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants