-
Notifications
You must be signed in to change notification settings - Fork 1
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 get_namespace_index to Session #16
Conversation
e73b140
to
d9f4a54
Compare
Okay, this works, but the session actually contains a place where we store a What if we did:
That does mean the user must make a dedicated call to fetch the namespace array, but it resolves the question of what the return type from |
I made get_namespace index update namespace in the background... not sure it is a good idea. sounds easier to me. but as you want. anyway who know what they do anyway would query directly the namespace map |
c8cead8
to
2039fa2
Compare
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.
This works, just some nitpicks.
We can add methods for getting directly from the cache without retrieving it from the server later, or make it visible in some way.
opcua-types/src/namespaces.rs
Outdated
@@ -26,6 +26,23 @@ impl NamespaceMap { | |||
} | |||
} | |||
|
|||
/// Create a new namespace map from a vec of variant as we get when reading | |||
/// the namespace array from the server | |||
pub fn new_from_variant_array(array: &[Variant]) -> Self { |
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.
This should probably return an error if any variant isn't a string, that is an error in this case.
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.
yes. I think I originally copied that code from somewhere. Can fix 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.
but once again I need to know what kind of error we should return.
If the project does not define errors, we need to make our own or decide to return the error with statuscode that is used several places
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.
In this case you can actually define a unit struct for it, or something like struct UnexpectedVariantType(usize)
containing the index of the variant that failed or something like that. That's what we do some other places.
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.
But simple enums/structs are not compatible with stderror and create a lot t of issues isn't it?
Then I would rather use thiserror crate which is anyway a dependency
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 made an MR: As far as I know this is the correct way to do it, but I do not have so much experience with rust
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.
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.
Looks good
@einarmo I pushed right after your approval... so you need to have a look and maybe approve again |
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.
Still fine, provided everything passes 👍
No description provided.