Kdb import export lookup provider through pluginDatabase #3180

MasterToney · 2019-11-08T18:45:36Z

Basics

These points need to be fulfilled for every PR:

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which
contains _(my name)_)
Please always add something to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy.
The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

If you are already Elektra developer:

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if the basics are fulfilled and you also
say that everything is ready to be merged.

With this PR kdb import and kdb export lookup providers through the database, before you had to specify the plugin to use directly by name, e.g. yajl where as now you can also specify the format e.g. json.

It will implement issue #1178.

MasterToney · 2019-11-08T19:08:09Z

@markus2330 please review my pull request.

markus2330 · 2019-11-08T19:43:40Z

Great job, looks very good. Let us see what the build server says.

MasterToney · 2019-11-08T19:47:31Z

Nice to hear, should I look into adding some unit tests that check if import/export work with both plugin name and file format?

markus2330 · 2019-11-08T19:54:48Z

Nice to hear, should I look into adding some unit tests that check if import/export work with both plugin name and file format?

Yes, some shellrecorder tests in the man pages of kdb import/export would be terrific.

markus2330 · 2019-11-08T19:57:38Z

Btw. if you plan to push any new commits on a PR, please remove "ready to merge". "ready to merge" indicate that you do not want to change anything anymore and the build server passes.

MasterToney · 2019-11-09T14:33:27Z

@markus2330 I added two tests for both import and export that check if the command works by specifying either the plugin directly or the desired format.
I also installed ronn and rebuilt elektra with it, the build tells me that the man pages are getting generated but the new examples are not included, so my question: are shell recorder examples excluded from the man pages automatically or am I missing something?

sanssecours · 2019-11-09T16:10:05Z

are shell recorder examples excluded from the man pages automatically or am I missing something?

Nope, these examples are included in the man pages. The problem is ElektraManPage, which tries to get rid of man pages, where only the date of the generated man page changed:

libelektra/scripts/cmake/ElektraManPage.cmake

Lines 35 to 41 in cac326c

    
           if (NOT "${DIFF_POSITION_DATE_ADDED}" STREQUAL -1 AND "${DIFF_POSITION_DATE_ADDED}" STREQUAL "${DIFF_LAST_POSITION_ADDED}") 
        
           	execute_process (COMMAND ${CMAKE_COMMAND} 
        
           				 -E 
        
           				 copy 
        
           				 ${OLD_MAN_PAGE_COPY} 
        
           				 ${MANPAGE}) 
        
           endif (NOT "${DIFF_POSITION_DATE_ADDED}" STREQUAL -1 AND "${DIFF_POSITION_DATE_ADDED}" STREQUAL "${DIFF_LAST_POSITION_ADDED}")

. Unfortunately I did not consider, changes which only include additions. As consequence these kind of man pages are generated, but then immediately reverted. Sorry for this bug. I will try to come up with a solution for this problem.

MasterToney · 2019-11-09T16:15:58Z

@sanssecours no problem ^^ I'm glad it's not something on my end :p
please let me know when a fix is available so that I can include the generated manpages

sanssecours · 2019-11-09T20:05:31Z

please let me know when a fix is available so that I can include the generated manpages

Thank you for your understanding. Pull request #3184 should fix the problem.

markus2330 · 2019-11-10T08:25:14Z

I merged #3184, so rebasing to master should fix the problem.

MasterToney · 2019-11-10T16:39:36Z

@sanssecours thank you for the fix, unfortunately now all man pages are marked as changed in doc/man/man1 and doc/man/man7

The difference in those files is as follows:

- .TH "ELEKTRA\-RELATED" "" "June 2019" "" ""
+ .TH "ELEKTRA\-RELATED" "" "October 2019" "" ""

I guess the script includes them since a deletion and insertion happened?
For me now it's not a big deal, I can reset them manually but might be something worthy to look into again.

sanssecours · 2019-11-10T20:47:23Z

@sanssecours thank you for the fix, unfortunately now all man pages are marked as changed in doc/man/man1 and doc/man/man7

I only tested my changes with macOS until now. I assume diff -e produces different output on your system. Which version of diff do you use? I now tried

diff (GNU diffutils) 2.8.1 on macOS, and
diff (GNU diffutils) 3.7 on Alpine Linux,

which both seem to work. The diff tool of BusyBox on the other hand does not work at all, since it does not support the POSIX option -e.

I guess the script includes them since a deletion and insertion happened?

Currently the CMake code just checks, if the diff output for the editor ed (diff -e) matches the regex ^4c\n\\.TH [^\n]+\n\\.\n$. If it does, then the code reverts the changes made by ronn.

MasterToney · 2019-11-10T21:28:46Z

I'm too running macOS with diff (GNU diffutils) 2.8.1 but strangely enough I wasn't able to reproduce the behavior after it's first occurrence.

partly implements #1178

verify that format and provider are accepted

partly implements #1178

… plugin

adds samples for the new functionality of #1178

markus2330 · 2019-11-11T10:50:11Z

jenkins build libelektra please

MasterToney · 2019-11-11T16:38:42Z

jenkins build libelektra please

markus2330 · 2019-11-11T16:52:08Z

Release notes are missing.

MasterToney · 2019-11-11T17:00:05Z

Are you sure? Jenkins logs say something along the lines that

508 [debian-buster-full] ERROR: Error fetching remote repo 'origin'
509 [debian-buster-full] hudson.plugins.git.GitException: Failed to fetch from https://github.com/ElektraInitiative/libelektra.git

which indicates to me that jenkins cannot clone this repo somehow.
Especially since I changed doc/news/_preparation_next_release.md with my contribution.

markus2330 · 2019-11-11T17:18:26Z

Are you sure?

I am sure my statement was wrong 😉, the problem seems to be somewhere else.

If retrying does not help, making a new PR usually helps.

MasterToney · 2019-11-11T17:23:39Z

Ok, I closed this one and created #3191, let's hope for the best ^^

MasterToney added the ready to merge label Nov 8, 2019

MasterToney added work in progress and removed ready to merge labels Nov 8, 2019

restyled-io bot mentioned this pull request Nov 9, 2019

Restyle Kdb import export lookup provider through pluginDatabase #3182

Closed

Anton Hößl added 11 commits November 10, 2019 22:32

tools(kdb-export): use provider database to lookup suiting plugins

963a086

partly implements #1178

tools(kdb-import): use provider database to lookup suiting plugins

65a86da

partly implements #1178

doc: add entry about kdb import/export to _preparation_next_release.md

03975fc

doc: add name to release note

e13e9d5

test(kdb-import): add shell-recorder tests

73bd081

verify that format and provider are accepted

test(kdb-export): add shell-recorder tests

971bcf3

verify that format and provider are accepted

styling: fix violations in kdb-export and kdb-import

372b3b9

tools(kdb-editor): use provider database to lookup suiting plugins

13a07f8

partly implements #1178

doc: update _preparation_next_release

fc7b8a5

doc(kdb-editor): add example for specifying the format instead of the…

e0e0fef

… plugin

doc(man1): update generated man pages for kdb- editor, export and import

34e6097

adds samples for the new functionality of #1178

MasterToney closed this Nov 11, 2019

MasterToney removed the work in progress label Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kdb import export lookup provider through pluginDatabase #3180

Kdb import export lookup provider through pluginDatabase #3180

MasterToney commented Nov 8, 2019 •

edited

Loading

MasterToney commented Nov 8, 2019

markus2330 commented Nov 8, 2019

MasterToney commented Nov 8, 2019

markus2330 commented Nov 8, 2019

markus2330 commented Nov 8, 2019

MasterToney commented Nov 9, 2019

sanssecours commented Nov 9, 2019

MasterToney commented Nov 9, 2019

sanssecours commented Nov 9, 2019

markus2330 commented Nov 10, 2019

MasterToney commented Nov 10, 2019 •

edited

Loading

sanssecours commented Nov 10, 2019

MasterToney commented Nov 10, 2019

markus2330 commented Nov 11, 2019

MasterToney commented Nov 11, 2019

markus2330 commented Nov 11, 2019

MasterToney commented Nov 11, 2019

markus2330 commented Nov 11, 2019

MasterToney commented Nov 11, 2019

Kdb import export lookup provider through pluginDatabase #3180

Kdb import export lookup provider through pluginDatabase #3180

Conversation

MasterToney commented Nov 8, 2019 • edited Loading

Basics

Checklist

Review

Labels

MasterToney commented Nov 8, 2019

markus2330 commented Nov 8, 2019

MasterToney commented Nov 8, 2019

markus2330 commented Nov 8, 2019

markus2330 commented Nov 8, 2019

MasterToney commented Nov 9, 2019

sanssecours commented Nov 9, 2019

MasterToney commented Nov 9, 2019

sanssecours commented Nov 9, 2019

markus2330 commented Nov 10, 2019

MasterToney commented Nov 10, 2019 • edited Loading

sanssecours commented Nov 10, 2019

MasterToney commented Nov 10, 2019

markus2330 commented Nov 11, 2019

MasterToney commented Nov 11, 2019

markus2330 commented Nov 11, 2019

MasterToney commented Nov 11, 2019

markus2330 commented Nov 11, 2019

MasterToney commented Nov 11, 2019

MasterToney commented Nov 8, 2019 •

edited

Loading

MasterToney commented Nov 10, 2019 •

edited

Loading