Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
addition of c# wrapper with libs for windows and macos #66
addition of c# wrapper with libs for windows and macos #66
Changes from 37 commits
3427272
79b7066
6f62f08
bb7d580
9caca00
cf9c860
6d63737
4b95b9a
9a259ef
70daffc
6458134
1c2da8b
34d7812
e0cac19
8b5a475
f868041
035a1bd
d334580
784aa2e
d093756
e0c5c8b
0a161b2
58b2c98
3a7deb2
b2b8aaa
bacf057
a9d8d35
fa17d16
9347f06
73b813e
ed76ec3
0f3615e
916bd57
a3ee3cc
66a042a
550cc02
2b0ea9c
0127302
9d35824
2b383b1
89f11c4
1b672ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end of the file. Github highlight it with a red flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to have these doc strings. However - I can see that the Java one doesn't have them and of course it would be nice not to duplicate them. Should we instead not have them here at all and document them in
ffi_wrapper.go
with at most links to the corresponding functions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - a C# developer will need to look at C# code.
Java isn't delivered to external developers so there's less emphasis on making it prettier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.. so in the future we'll be maintaining doc-strings (which should all the same thing) in rust, Java & C#. (I guess that's unavoidable.)
Good point about Java - should we mark that as "beta" / "unsupported" then? (The repo after all is public - how would people distinguish what is ready and what is not? 🤔 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Tools.Ensure*
usage: Surely the golang layer should be performing validation like this and if not we should add it and keep the wrappers as thin as possible. (The extra logic conditions would all need tests to verify)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the golang validation kicks in if the # isn't present. I don't know why the library doesn't do it.
it's a bloody pain - so I decided to add it into the application layer.
but I choose not to touch the golang layer including ffi.
I am happy to remove it if it's a bother but then we need to document it somewhere for the users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also - validation and input sanitisation should be done as early as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but C#/Java/FFI layers should be very thin so I'm not sure I agree that this means it should happen there. What's wrong with "throwing" things early on behind the actual golang API itself?
But I appreciate that's not done yet. This sounds like an improvement to make soon (but obv. not now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tools.Ensure*
(I know I mentioned it elsewhere already, but) wouldn't it be better to just pass through and let the golang lib validate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I repeatedly endup writing the ensure in the application - i find it counter intuitive to have to prepend the string with a #
I am not strong about it but if you prefer not to do it in the Golang layer then we need to document it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I worded that badly - I am suggesting we do validate extra stuff in golang level (not even FFI) so that each of the FFI wrappers can be thinner and validation is consistent across languages.