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

Serialize all responses used in the NEST Server #2635

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

babsey
Copy link
Contributor

@babsey babsey commented Mar 13, 2023

It shows a usecase that NodeCollection or other NEST objects are stored in userdict.
Then the user wants to get userdict via API-requests.

Example:

curl localhost:52425/api/userdict

It produces server error because the data in userdict cannot be transformed for JSON when they are NodeCollection or other NEST objects.

@babsey babsey requested a review from jougs March 13, 2023 15:03
@babsey babsey changed the title Serializable all responses Serialize all responses Mar 14, 2023
@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label May 14, 2023
@terhorstd terhorstd added this to In progress in PyNEST via automation Jun 27, 2023
@terhorstd terhorstd added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: External API Developers of extensions or other language bindings may need to adapt their code and removed stale Automatic marker for inactivity, please have another look here labels Jun 27, 2023
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

LGTM! Could you please merge with master so that the new CI pipeline can run?

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

The changes in this PR look good to me.

Just one thing that always bothered me: Why is the function called serializable()? From a function with that name I would expect that it returns a Boolean flag indicating if the argument can be serialized or not. But this function actually serializes its argument and consequently should rather be called serialize()? Shouldn't it?

Or am I missing something essential here?

@babsey
Copy link
Contributor Author

babsey commented Aug 29, 2023

Is serialize_data better suitable for it?

@jougs
Copy link
Contributor

jougs commented Aug 29, 2023

Much better in my opinion!

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Beautiful! ❤️

@jougs jougs merged commit 69eb261 into nest:master Aug 29, 2023
20 checks passed
PyNEST automation moved this from In progress to Done Aug 29, 2023
@jougs jougs changed the title Serialize all responses Serialize all responses used in the NEST Server Aug 29, 2023
@babsey babsey deleted the nest-server-serializable branch August 29, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: External API Developers of extensions or other language bindings may need to adapt their code S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
PyNEST
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants