-
Notifications
You must be signed in to change notification settings - Fork 123
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
Kdb rewrite base #4943
Kdb rewrite base #4943
Conversation
edf8782
to
93485a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of using macros when you don't need to. I left some comments about that, but IMO we could also merge this with the macros as they are now, as long as they are better documented (especially hidden assumptions).
The reason looks all very good. I just have some minor suggestions.
I also had an idea how to improve the integration of the old C++ code, but again we can also leave it the way it is now, as long as it's documented properly. Specifically, about the external shell script commands, the docs should state whether that is supported only via C++ or even, if you replace cpp_main
with a dummy so you can compile without C++.
4bd6d67
to
53f9bb0
Compare
Thanks a lot for the feedback! |
b51edcd
to
026b455
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a two minor inconsistencies/mistakes, please check. Otherwise, LGTM.
026b455
to
813f9c7
Compare
c00b8cb
to
ef566b6
Compare
jenkins build libelektra please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, just some small improvements to the new docs.
499db7d
to
f14de05
Compare
jenkins build libelektra please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
21193e3
to
f14de05
Compare
and add command.h containing helpful macros and a struct def that is needed later. Commands not implemented yet will be handles by the old implementation.
... to command.c for resolving bookmarks in keynames.
... remove cpp implementation and add test for the get command
... based on @kodebach 's feedback
... based on @markus2330 's feedback
0f7b532
to
fe6ba2e
Compare
I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new PR with the remainder of this PR. |
I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new PR with the remainder of this PR. |
This is the "framework" for the new
C
cli.I'll mention PRs for commands later here.
This can be merged as is, more commands can be added later.
add spec support for external commands: Kdb rewrite external #4951 (reviewed and now inkdb_rewrite_base
)add spec for: Kdb rewrite cpp spec #4949 (reviewed and now inC++
commandskdb_rewrite_base
)add set command: Kdb rewrite set #4954 (reviewed and now inkdb_rewrite_base
)Basics
(added as entry in
doc/news/_preparation_next_release.md
which contains_(my name)_
)Please always add them to the release notes.
(first line should have
module: short statement
syntax)close #X
, are in the commit messages.doc/news/_preparation_next_release.md
scripts/dev/reformat-all
Checklist
(not in the PR description)
Review
Labels