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 without master #14

Merged
merged 22 commits into from
Nov 24, 2019
Merged

Conversation

FelixResch
Copy link
Collaborator

New PR without the changes from upstream master.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Amazing progress. I think we should merge this soon, create issues with the TODOs and then make a follow-up PR with the requested changes/new features.

.gitignore Outdated
@@ -13,9 +12,12 @@
.*.swp
.swp.*
Doxyfile
Makefile
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not remove something here, this only leads to unnecessary discussions.

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 have added the rules back in, i think they were removed automatically by a plugin i use


ecm_add_test(
kconfigelektratest.cpp
../src/core/kconfigelektra.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these sources required? It would be better if we can test directly against the public interfaces.

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 need those sources to test the backend directly

@@ -50,6 +50,28 @@ ecm_add_tests(
LINK_LIBRARIES KF5::ConfigCore Qt5::Test Qt5::Concurrent
)

find_package(Elektra)
Copy link
Contributor

Choose a reason for hiding this comment

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

add to beginning where the other find_packages are.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -32,7 +32,7 @@ private Q_SLOTS:

void FallbackConfigResourcesTest::initTestCase()
{
QStandardPaths::setTestModeEnabled(true);
QStandardPaths::setTestModeEnabled(true); //TODO create and drop expected data (i guess)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better if you create issues and not TODOs or FIXMEs.

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 will create the issues today in the afternoon

#include <iostream>
#include "kconfigelektratest.h"

#ifdef FEAT_ELEKTRA
Copy link
Contributor

Choose a reason for hiding this comment

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

better to exclude the whole file in CMake

* @param ks
* @return
*/
static std::string findLowestKeyspace(KeySet* ks)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be automatically done when you use cascading lookups. (lookup key starts with /)

const KEntryKey &entryKey = it.key();
KEntry entry = it.value();

if (entryKey.mKey.isEmpty()) { // ignore group attributes for the moment
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this is (more or less) part of #10, to write the group attributes to the files again


#include <kdb.hpp>

using namespace kdb;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be done only in source files

//
// Created by felix on 28.10.19.
//
#include "kconfigbackend_p.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

why _p?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Header files with the postfix _p are apparently removed from header files and not installed to the include directory. This way the user cannot use internals of the library.


#endif //KCONFIG_KCONFIGELEKTRA_P_H

#endif //FEAT_ELEKTRA
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid including the whole file via cmake

@FelixResch FelixResch mentioned this pull request Nov 23, 2019
@FelixResch
Copy link
Collaborator Author

Can I merge this now?

@markus2330 markus2330 merged commit 7aa6f44 into master Nov 24, 2019
@markus2330 markus2330 deleted the elektra-backend-without-master branch November 24, 2019 14:57
@markus2330
Copy link
Contributor

Done.

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