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

Update carlos pr #128

Merged
merged 37 commits into from
Dec 7, 2022
Merged

Conversation

jverswijver
Copy link
Contributor

add the final touches to #127 since @zitrosolrac is unavailable

@guzman-raphael
Copy link
Collaborator

Supersedes #127

Copy link
Collaborator

@guzman-raphael guzman-raphael left a comment

Choose a reason for hiding this comment

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

👏 Thanks @jverswijver @zitrosolrac! 👏 This is looking great and DS team is looking forward to it.

Just have a few remaining items to clear up before we are ready to merge.

pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Show resolved Hide resolved
pharus/component_interface.py Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/version.py Outdated Show resolved Hide resolved
tests/test_api_gen.py Outdated Show resolved Hide resolved
tests/test_api_gen.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
@guzman-raphael guzman-raphael mentioned this pull request Aug 29, 2022
Copy link
Collaborator

@guzman-raphael guzman-raphael left a comment

Choose a reason for hiding this comment

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

@jverswijver Thanks for refactoring to address the distinct fields included in the attributes. 🤝

There was some feedback from my last review that is still unresolved. Providing additional feedback here from the new review. Let me know if you have any questions or concerns.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pharus/dynamic_api_gen.py Outdated Show resolved Hide resolved
tests/test_api_gen.py Outdated Show resolved Hide resolved
pharus/interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Show resolved Hide resolved
@jverswijver
Copy link
Contributor Author

@guzman-raphael ended up figuring out why the get_json method was not working in the tests without force=True, it seems like we were never setting the mimetype and just letting the users browser figure it out. This works with modern browsers like chrome because they apparently dont rely on the mimetype field and instead infer the mimetype from the data directly. To fix this in the tests I have added a property to the classes in the component interface which allows you to specify the mimetype.

A-Baji
A-Baji previously approved these changes Nov 30, 2022
A-Baji
A-Baji previously approved these changes Dec 7, 2022
A-Baji
A-Baji previously approved these changes Dec 7, 2022
@guzman-raphael guzman-raphael merged commit 0cdf1b0 into datajoint:master Dec 7, 2022
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.

None yet

4 participants