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

implement external iterators #3171

Open
markus2330 opened this issue Nov 7, 2019 · 11 comments
Assignees

Comments

@markus2330
Copy link
Contributor

@markus2330 markus2330 commented Nov 7, 2019

As written in #2991 we want to switch to external iterators. Which bindings do still make use of the internal iterators (ksNext and friends?). They should be removed/deprecated and an external iterator provided instead.

  • Plugins (need rewind)
  • Java (remove internal)
  • Python
  • Lua
  • Rust
  • Go
  • GLIB
  • CPP (update docu: deprecated)
  • Ruby
@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 7, 2019

Both python and lua are using the CPP NameIterator and KeySetIterator. So if you remove them you are also removing them from python+lua.

@manuelm manuelm removed their assignment Nov 7, 2019
@Piankero

This comment has been minimized.

Copy link
Contributor

@Piankero Piankero commented Nov 7, 2019

ksNext and friends?

What is "friends" in here?

The java implementation already uses an external iterator

Once ksNext() is removed, this line should also be removed.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 7, 2019

@Piankero I assume ksRewind, ksNext, ksCurrent, ksGetCursor, ksSetCursor

@markus2330 You should add Ruby. It's actually wrapping all five and also using them in the ruby iterator implementation

@manuelm manuelm referenced this issue Nov 7, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 7, 2019

Thank you for the hint, I added Ruby (@BernhardDenner).

@Piankero can you fix Java, it should be quite easy for you to write an iterator?

@Piankero

This comment has been minimized.

Copy link
Contributor

@Piankero Piankero commented Nov 7, 2019

@Piankero can you fix Java, it should be quite easy for you to write an iterator?

There is already an external iterator written as I have written in the upper comment.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 7, 2019

Even better, then you only need to remove the internal iterator, I updated the description above.

@Piankero

This comment has been minimized.

Copy link
Contributor

@Piankero Piankero commented Nov 8, 2019

@manuelm @markus2330 Are you sure about the ksRewind? If I remove this line I receive test failures in the integration tests of jna.

Piankero added a commit to Piankero/libelektra that referenced this issue Nov 8, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 8, 2019

Yes, the tests also need update.

@Piankero

This comment has been minimized.

Copy link
Contributor

@Piankero Piankero commented Nov 8, 2019

Yes, the tests also need update.

I already have updated some test cases but the set/get methods seem to need a rewinded keyset. what should I do if I cannot rewind it anymore?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 8, 2019

Thank you this is really a good point. I am afraid we need to fix all plugins that use the internal cursor to properly rewind before using the iterator.

We also need some docu (in the release notes) how to use the external iterator and how to migrate away from the internal iterator:

  1. we only use external iterator from now on.
  2. after 1.0 release we slowly start replacing usage of internal iterator (to use external iterator instead)
@markus2330 markus2330 referenced this issue Nov 11, 2019
0 of 5 tasks complete
@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 11, 2019

ksGet/SetCursor is also a friend of ksNext, I opened #3189 for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.