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

remove kdbproposal.h #2993

Open
markus2330 opened this issue Sep 20, 2019 · 15 comments

Comments

@markus2330
Copy link
Contributor

commented Sep 20, 2019

  • keySetStringF move to internal?
  • elektraKsToMemArray completely remove?
  • ksRenameKeys completely remove?
  • keyLock and elektraLockOptions: move to internal?
  • elektraLookupOptions
  • elektraKeySetName: use instead of keySetName or always allow meta names?
  • elektraKeyGetMetaKeySet see #2983
  • ksPrev and ksPopAtCursor: needs discussion
  • keyRel2 better than keyRel, but maybe remove both?
  • keyAsCascading remove?
  • keyGetLevelsBelow?
@markus2330 markus2330 added the cleanup label Sep 20, 2019
@markus2330 markus2330 added this to the 0.9.* milestone Sep 20, 2019
@PhilippGackstatter PhilippGackstatter self-assigned this Oct 3, 2019
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

I moved keyLock and elektraLockOptions to kdbprivate, because elektraKeyLock depends on them, and since kdbinternal depends on kdbprivate, the first two cannot live there.

elektraKsToMemArray has 13 references, such as in libs/meta/meta.c, plugins/ini/ini.c, plugins/keytometa/keytometa.c, and more. Should this really be removed?

ksRenameKeys has one reference in plugins/csvstorage. I can move the function into the plugin, and then delete it in all other places.

elektraLookupOptions isn't used anyhwere atm. It also seems it won't have a use in #2983, so should this be removed?

keyAsCascading has only usage in keyRel2, so can be removed if we remove keyRel2.
Same for keyGetLevelsBelow.
Even if we keep keyRel2, I suppose these functions can be made private if there's no need for them elsewhere.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

I moved keyLock and elektraLockOptions to kdbprivate, because elektraKeyLock depends on them, and since kdbinternal depends on kdbprivate, the first two cannot live there.

Good.

elektraKsToMemArray has 13 references, such as in libs/meta/meta.c, plugins/ini/ini.c, plugins/keytometa/keytometa.c, and more. Should this really be removed?

No, then it should be moved to libease (libmeta depends on libease anyway, for the plugins it is no problem to depend on it)

ksRenameKeys has one reference in plugins/csvstorage. I can move the function into the plugin, and then delete it in all other places.

Yes.

elektraLookupOptions isn't used anyhwere atm. It also seems it won't have a use in #2983, so should this be removed?

Move it to kdbprivate (it is internally in ksLookup used).

keyAsCascading has only usage in keyRel2, so can be removed if we remove keyRel2.
Same for keyGetLevelsBelow.

#3034

Even if we keep keyRel2, I suppose these functions can be made private if there's no need for them elsewhere.

Yes, I agree. You can add a static.

@markus2330 markus2330 referenced this issue Oct 4, 2019
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

ksRenameKeys has one reference in plugins/csvstorage. I can move the function into the plugin, and then delete it in all other places.

Yes.

I missed that ksRenameKeys is just a wrapper for elektraRenameKeys which is used in a number of plugins. Is this another candidate for elektra-ease then? I suppose we can get rid of the wrapper?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2019

Yes, the wrapper is not needed. If the core does not need it, then ksRenameKeys should be moved to elektra-ease.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2019

Moving ksRenameKeys from core to ease works fine.
Where should elektraRenameKeys be? It's currently in core (implemented in proposal.c) and moving it to ease is not trivial, since there are quite a few places where ease would have to be linked additonally.

The real question is, whether we should get rid of proposal.c as well, which only contains 4 functions (including elektraRenameKeys) and where those functions should move.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2019

Where should elektraRenameKeys be?

Nowhere as we want to get rid of this wrapping. There should be only ksRenameKeys (or elektraRenameKeys if it fits better to the other functions in libease) and nothing else.

are quite a few places

Which one? Only plugins and tools? Then it would be no problem.

The real question is, whether we should get rid of proposal.c as well

Yes, the file should be removed.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

Which one? Only plugins and tools? Then it would be no problem.

Moving elektraRenameKeys to ease produces a lot of
/usr/bin/ld: ../lib/libelektra-kdb.so.0.9.0: undefined reference to elektraRenameKeys errors.

src/libs/kdb.c (and mount.c, backend.c and plugin.c) for instance need elektraRenameKeys, so it seems to belong to the core for that reason.
I can of course link libelektra-kdb against ease to take care of that, but that's a dependency for just one function.

If we leave it in core, then to which file should this be moved?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

There seems to be an easy solution: we simply move it to elektra-kdb. list+multifile already links against it. (@vLesk might move it to the backend plugin if possible.)

I would like to avoid having it in the core because it does not do anything that cannot easily programmed with the core API.

A int ksRenameKeys(KeySet * ksToBeRenamed, char * name) could be interesting as it is currently not possible to rename keys in a keyset. With such an API we could solve #404, as ksRenameKeys could also avoid making the keys dirty. This is something which cannot be done with the public API.

@vLesk

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

The implementation of the backend plugin moves the usage of elektraRenameKeys from plugin.c and backend.c to the backend plugin, but it is still used in kdb.c by the kdbEnsure function and in mount.c by elektraMountGlobalsGetConfig, which are not directly related to the backend plugin, so I think that moving it to somewhere like elektra-kdb would be a better idea.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

@vLesk Thank you for the quick reply. Maybe the backend plugin needs to link against elektra-kdb anyway? If it is only about elektraRenameKeys, it is better if you copy that one function to have the backend plugin with clean dependencies.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2019

Do you want to me to (try) move it to elektra-kdb?

Regarding ksPopAtCursor and elektraKsPopAtCursor: They seem to have some (around 6-7) usages. Same goes for ksPrev and elektraKsPrev, although fewer usages.
Apparently they will become obsolete once we have external iterators (#2991), according to the warning on ksPopAtCursor?

I suppose we can either move them to some temporary location or replace the current occurences now and get rid of them.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

Do you want to me to (try) move it to elektra-kdb?

Yes, but maybe in a different PR, to do everything of #2993 is too big.

ksPopAtCursor

This is actually (maybe) a part of what the external iterator needs (if the iterator also has some pop functionality).

ksPrev/elektraKsPrev

Yes, we can remove that. This is only for the internal iterator.

@PhilippGackstatter PhilippGackstatter referenced this issue Oct 8, 2019
4 of 16 tasks complete
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Yes, but maybe in a different PR, to do everything of #2993 is too big.

Moving elektraRenameKeys to elektra-kdb worked great, everything still compiles and links.
I just can't find a good place for the function in the files belonging to that library, where it makes sense to live. Do you have a suggestion?

The declaration I would leave in kdbprivate.h.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

I suppose elektraKeyGetMetaKeySet will be moved from kdbproposal.h somewhere else in #2983?

The same then goes for ksPopAtCursor which may become part of #2991.

elektraKeySetName is basically the implementation of keySetName, so it should either be removed and directly implemented in keySetName or moved to kdbprivate.h?

Once these are taken care of, this issue is done.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

I just can't find a good place for the function [ksRenameKeys] in the files belonging to that library, where it makes sense to live. Do you have a suggestion?

In the beginning of src/libs/elektra/kdb.c there is already another helper function, maybe there?

I suppose elektraKeyGetMetaKeySet will be moved from kdbproposal.h somewhere else in #2983?

Actually you can simply rename the function and move it to the core. The function is already needed by merging code and for bindings.

The same then goes for ksPopAtCursor which may become part of #2991.

Yes, here we need a bit more of discussion, maybe we do not need ksPopAtCursor at all.

elektraKeySetName is basically the implementation of keySetName, so it should either be removed and directly implemented in keySetName or moved to kdbprivate.h?

I created #3050 for discussion.

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