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

Elektra backend #13

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Elektra backend #13

wants to merge 20 commits into from

Conversation

FelixResch
Copy link
Collaborator

Master from upstream is already merged. This was necessary, as there were breaking changes in KF5 which were already used by other applications.

Code has been formatted according to KDE specification (https://community.kde.org/Policies/Frameworks_Coding_Style)

There currently are some dirty hacks in it, but the basic system is now working.

@markus2330
Copy link
Contributor

markus2330 commented Nov 20, 2019

Can you please properly pull to master (from upstream master), rebase your branch and create a new PR? The PR should only contain your changes.

@FelixResch
Copy link
Collaborator Author

Okay, i'm currently pushing a new branch and will open the pr shortly.

I had to pull in the upstream master, because they added some changes on which other applications depended. But this should work, once we merge with our master.

@markus2330
Copy link
Contributor

I had to pull in the upstream master, because they added some changes on which other applications depended. But this should work, once we merge with our master.

Yes, you can and should pull upstream master. But please push it directly to our master, not within your PR.

* @param parent the iterator of the parent key
* @param child the iterator of the child key
*/
inline void traverseIterators(Key::iterator* parent, Key::iterator* child) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameters can be passed as reference instead of pointer

std::string key;
};

inline KConfigKey elektraKeyToKConfigKey(Key::iterator* key, Key::iterator* end) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're not intending to manipulate an existing iterator, it would be safer to pass as value. Key::iterator (or NameIterator) only contains 3 pointers, so it won't introduce any noticeable overhead

Key key;


while((key = keySet.pop()) != nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can for(auto& key : keySet) be used here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change it to const but it works (for(const auto& key : keySet))

return ParseOk;
}

inline std::string kConfigGroupToElektraKey(std::string group, const std::string& keyname) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might introduce some trouble. In kconfig config files key names may contain the / character. I used ckdb::Key.setBaseName to append key/group names to a ckdb::Key so that it escapes characters automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, changed it

I pushed the changes to the branch of #14

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe lets merge #14 and close this PR. It is a bit confusing now.

@markus2330
Copy link
Contributor

@FelixResch please try not to keep important PRs open too long as this hinders others to successfully work with the master of this repo.

What is the status here? Can you please rebase?

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.

3 participants