Skip to content

Perf batch 1#656

Draft
timcadman wants to merge 3 commits intov7.0-devfrom
perf-batch-1
Draft

Perf batch 1#656
timcadman wants to merge 3 commits intov7.0-devfrom
perf-batch-1

Conversation

@timcadman
Copy link
Contributor

No description provided.

@timcadman timcadman marked this pull request as draft March 12, 2026 18:11
@StuartWheater
Copy link
Member

I am a bit concerned that the "tidying of the code", for example removing spaces, could cause problems when we come to merge v6.3.6-dev, v6.4.0-dev, ...., lots of conflicts. Could these changes be moved to a PR on v6.3.6-dev, moving them earlyed in release history.

I notices at the tests don't check that the results for the call which don't now return anything, I think that should check there are any results as intended.

@timcadman
Copy link
Contributor Author

Thanks Stuart, this is still draft so I was going to work a bit more before asking for your review! I will revert any changes which change whitespace etc. I also need to check tests further.

@StuartWheater
Copy link
Member

I also need to check tests further.
But if it did return a results wouldn't that be incorrect behaviour? So should be checked (I will generally put that in a "test-arg-..." test).

@timcadman
Copy link
Contributor Author

It should return a result, but we can check that the serverside object has been created correctly? I will look today

@StuartWheater
Copy link
Member

It would be an improvement if assignments could return a limited outcome.

@timcadman
Copy link
Contributor Author

timcadman commented Mar 13, 2026 via email

@timcadman
Copy link
Contributor Author

My thought process for this was that with R, you don't receive a message confirming that the process has worked. You just receive an error message. So I was thinking we do the same with DataSHIELD - let the server handle/return any errors, and if nothing is returned then it indicates the operation was successful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants