Renamed ping to info in store #8975

vicky1999 · 2023-09-13T10:33:57Z

Motivation

I have renamed the command store ping to store info

Context

This change is required in #8914

Changes made:

First, in main.cc, I have updated the command from ping-store to info-store and accordingly changed the command values
In info-store.cc, command was changed to info
Renamed ping-store.cc to info-store.cc
Renamed store-ping.sh to store-info.sh
Updated the doc info-store.md
To display true/false value for trusted, I made a condition if trusted has any value other than 0/false, then display true or else display false. After making this change, the json and store info is giving true/false for trusted instead of int

Screenshot

Priorities

Add 👍 to pull requests you find important.

thufschmitt

Thanks, left a couple of comments, but looks good overall. Great to see someone help tacking this :)

thufschmitt · 2023-09-13T11:58:51Z

src/nix/main.cc

@@ -126,7 +126,7 @@ struct NixArgs : virtual MultiCommand, virtual MixCommonArgs
        {"ls-store", {"store", "ls"}},
        {"make-content-addressable", {"store", "make-content-addressed"}},
        {"optimise-store", {"store", "optimise"}},
-        {"ping-store", {"store", "ping"}},
+        {"info-store", {"store", "info"}},


That one shouldn't be changed. The ping-store alias should stay the same.
We should also have a backwards-compat alias from store ping to store info I think. @Ericson2314 @edolstra is there a mechanism for aliasing subcommands? If not we can always do it manually

Thanks. So, According to the comment, the ping-store need to stay the same but, the vector alias of it {"store", "info"} can be as it is. please let me know if this is the expectation

I do not off-hand know of a mechanism for aliasing within subcommands, but if we look how this is implemented with overriding rewriteArgs, perhaps that can be done within the Store command.

I have changed the store command in store-info.cc. After that, I had a suggestion that, if we are renaming the command from ping to info, then I thought it would be better to also update the alias accordingly.

I am not sure what you are saying? I am saying you can modify src/nix/store.cc to that nix store ping is an alias for nix store info in addition.

Please let me know if there are any documentations available for this aliasing.

There is no documentation for this exact task. You would need to figure out what main.cc is doing and recreate it in store.cc. I am sorry.

It would be good to add API docs to rewriteArgs where it is declared (args.hh). Perhaps once you've figured out what is going on your can do that, so the next person is less stuck?

Sure. I will try to implement it in store.cc. Thanks.

thufschmitt · 2023-09-13T12:00:22Z

src/nix/info-store.cc

-        return
-          #include "ping-store.md"
-          ;
+        return #include "info-store.md"


Not a big deal (and no blocker to me), but since we're renaming things, we could rename this to store-info.md to match the command (ping-store comes from the original command name and was already out of sync)

store-info files are renamed according to the command name

I think what @thufschmitt is saying is that it would be good to call both files store-info.{cc,md}. The file was never properly renamed when the commands were made hierarchical before, but we should fix that.

Sure. I will try to implement the aliasing in store.cc. Thanks

Ericson2314 · 2023-09-14T06:35:04Z

src/nix/store-info.cc

@@ -41,9 +41,9 @@ struct CmdPingStore : StoreCommand, MixJSON
            if (auto version = store->getVersion())
                res["version"] = *version;
            if (auto trusted = store->isTrustedClient())
-                res["trusted"] = *trusted;
+                res["trusted"] = *trusted ? true : false;


We should eventually not need this via the enum struct change I proposed for TrustedFlag, but it is fine to do this for now and that in a follow-up PR.

sure. I saw that TrustedFag is storing only bool value. But, somehow the value is printing 1/0 instead of true or false. So, I added this condition here.

It's because it is the wrong type of enum. The other type of enum will remove the implicit coercions that are causing the problem here.

vicky1999 · 2023-09-14T08:52:54Z

Another update regarding the store info --json command. I have made the JSON to be displayed with formatting.

vicky1999 · 2023-09-18T06:34:06Z

@Ericson2314 I tried to add the aliasing in store.cc. But, it seems that, args.cc is throwing an error if no subcommand exist matching the list of subcommands. So, I made some changes to args.cc and made the alias implementation under args.cc. Need your suggestion on this.

Kindly please review the commit: fc094c1 and let me know if it is ok.

Ericson2314 · 2023-09-19T19:50:22Z

src/nix/store-info.cc

        } else {
            nlohmann::json res;
            Finally printRes([&]() {
-                logger->cout("%s", res);
+                logger->cout("%s", res.dump(2));


What is this change? formatting the JSON?

This change is about formatting the json like in the screenshot below:

Without the change, the json looked like:

{"trusted": true, "url": "local", "version": "2.1.0"}

I added the code to view the json with some indents ans spaces to give a better to view for this json command.

We do not usually format output json, so I for consistency I don't think we want to here either. If we decide to do so, it should be across the board not a one-off decision for one command.

ok. I will revert the changes

@Ericson2314 the changes related to aliasing which was done in args.cc and store info json formatting were reverted. i will try the new approach you mentioned for aliasing in main.cc.

Ericson2314 · 2023-10-09T12:16:27Z

@vicky1999 See CmdFlakeInfo for an existing alias/deprecation we have. It's not a great way to do it but we can just do it that way for now.

vicky1999 added 7 commits September 13, 2023 08:46

ping renamed to info

5cb0bf8

testcases updated for store ping

ecde425

formatting changes

17619b2

test file renamed

6973044

reverted ping store alias

91b314e

store info files renamed

8f4557b

changes to display trusted as bool instead of number

1a4f4f9

vicky1999 requested review from fricklerhandwerk and edolstra as code owners September 13, 2023 10:33

github-actions bot added documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Sep 13, 2023

vicky1999 added 2 commits September 13, 2023 16:40

info store command formatting fixes

7fcc122

formatting chagnes to resolve doc error in store info

0333d18

thufschmitt suggested changes Sep 13, 2023

View reviewed changes

vicky1999 requested a review from thufschmitt September 13, 2023 13:37

vicky1999 mentioned this pull request Sep 14, 2023

Stabilise nix store ping #8914

Closed

Ericson2314 reviewed Sep 14, 2023

View reviewed changes

fricklerhandwerk added the RFC Related to an accepted RFC label Sep 14, 2023

store info json display will be indented

0652e26

thufschmitt assigned Ericson2314 Sep 15, 2023

thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Sep 15, 2023

vicky1999 added 2 commits September 18, 2023 11:57

alias implementation in args

fc094c1

comments added

0556de9

Ericson2314 reviewed Sep 19, 2023

View reviewed changes

reverted aliasing in args.cc and json formatting for store info

7fd7db7

Ericson2314 mentioned this pull request Oct 9, 2023

Command alias implementation #9127

Closed

vicky1999 closed this by deleting the head repository Oct 10, 2023

vicky1999 mentioned this pull request Oct 13, 2023

nix store ping -> nix store info #9150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed ping to info in store #8975

Renamed ping to info in store #8975

vicky1999 commented Sep 13, 2023 •

edited

Loading

thufschmitt left a comment

thufschmitt Sep 13, 2023

vicky1999 Sep 13, 2023

Ericson2314 Sep 14, 2023

vicky1999 Sep 14, 2023

Ericson2314 Sep 15, 2023

vicky1999 Sep 16, 2023

Ericson2314 Sep 17, 2023

vicky1999 Sep 17, 2023

thufschmitt Sep 13, 2023

vicky1999 Sep 13, 2023

Ericson2314 Sep 17, 2023

vicky1999 Sep 17, 2023

Ericson2314 Sep 14, 2023

vicky1999 Sep 14, 2023

Ericson2314 Sep 14, 2023

vicky1999 commented Sep 14, 2023

vicky1999 commented Sep 18, 2023

Ericson2314 Sep 19, 2023

vicky1999 Sep 20, 2023

Ericson2314 Sep 20, 2023

vicky1999 Sep 20, 2023

vicky1999 Sep 20, 2023

Ericson2314 commented Oct 9, 2023

Renamed ping to info in store #8975

Renamed ping to info in store #8975

Conversation

vicky1999 commented Sep 13, 2023 • edited Loading

Motivation

Context

Changes made:

Screenshot

Priorities

thufschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicky1999 commented Sep 14, 2023

vicky1999 commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Oct 9, 2023

vicky1999 commented Sep 13, 2023 •

edited

Loading