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

fix: Wrong shelf loading #1578

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

AiyionPrime
Copy link
Contributor

@AiyionPrime AiyionPrime commented Feb 13, 2024

blocked by #1577

So this is the last and most controversial PR, due to the last commit.
This simply removes the "NotImplementedError` which currently guards against usage of the lazydict.
I'm not saying this would be a good solution, I just wanted to have a commit to run tests from and thought I'd share it with you.

Eventually this resolves #867 as the typehints clarify how to use the shelf file.

@AiyionPrime AiyionPrime force-pushed the fix/wrong_shelf_loading branch from e3daf33 to 4755dde Compare February 13, 2024 15:04
@oroulet
Copy link
Member

oroulet commented Feb 16, 2024

as I wrote in another PR, I have never usef that feature so I can merge whatever you think is useful, as long as you do not break things ;-)

@oroulet
Copy link
Member

oroulet commented Feb 16, 2024

changes are quite small and I did not see any obvious big coding errors

@AiyionPrime AiyionPrime force-pushed the fix/wrong_shelf_loading branch from 4755dde to ed714e2 Compare February 20, 2024 10:06
@AiyionPrime AiyionPrime marked this pull request as ready for review February 20, 2024 10:06
@AiyionPrime
Copy link
Contributor Author

@oroulet Last but not least :)
The only controversial commit is the last one, where I remove the NotImplementedError which @cbergmiller put in place years ago.

The idea is, that the current shelve implementation is good for synchronous usage, but not async friendly as he put it.

My take on this is, the feature does work in some circumstances and wont benefit asynchronous ones as much, if at all; but that's for the user to assess. Until someone comes up with a better and async oriented approach this is still beneficial for synchronous users.

And as the feature is not on by default this is not going to make matters worse for anyone.

I'd say this is good to go, once the pipe's green.

@oroulet oroulet merged commit f1f961f into FreeOpcUa:master Feb 21, 2024
5 checks passed
@AiyionPrime AiyionPrime deleted the fix/wrong_shelf_loading branch February 21, 2024 19:42
@AiyionPrime
Copy link
Contributor Author

@oroulet Nice, thank you!

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.

How to pass the cache file to the server
2 participants