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

refactor(cmd): extract rpc cmds #2706

Merged
merged 10 commits into from
Sep 25, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Sep 15, 2023

Overview

Tested cmds against arabica-10 and mocha-4

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

❗ No coverage uploaded for pull request base (rpc_cmds_feature_branch@14ea252). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                     @@
##             rpc_cmds_feature_branch    #2706   +/-   ##
==========================================================
  Coverage                           ?   50.52%           
==========================================================
  Files                              ?      166           
  Lines                              ?    10764           
  Branches                           ?        0           
==========================================================
  Hits                               ?     5439           
  Misses                             ?     4857           
  Partials                           ?      468           

Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

made a few organization suggestions, mostly around seeing some repeated stuff and for a nicer looking package name. We'll revisit after discussing/changing those.

I'd also suggest maybe adding a internal/formatting/formatting.go package, and replacing internal.PrintOutput with something like commands/formatting -> formatting.PrintOutput

anything that reduces the need for maintain mental context around what internal is and where something might be coming from is my preference and having to "think upwards" to the import

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

We don't use internal anywhere yet, while we should. There are tons of internals we have that are supposed to be hidden away, and I don't think cmds are one of them. I propose to move the internalization to a separate issue and look at it holistically, what should be hidden in the whole project and what's not. We would also discuss there why we would wanna hide away cmds for our modules. Right now, I am not convinced that that's something we should do atm.

I propose to go with either of the structures here:

  • cmd/cli/<mod>/... or cmd/<mod>/...
  • nodebuilder/<mod>/cmds/...

I personally prefer the latter as its brings us closer to the modular node vision

@vgonkivs
Copy link
Member Author

vgonkivs commented Sep 18, 2023

Yes, we don't use internal anywhere in our node's code. But why can't we do it now for cmds? Anyway, this is a global refactoring of cmd package. Why can't we make it as it should be now and not in the future? I don't like when smth that is not related to the codebase could be used wherever. If cmds are supposed to be used only inside the cmd/ why shouldn't they be hidden from other packages?
IMO playing around with the restrictions(where necessary) helps to keep code cleaner and easier to read for newcomers

@vgonkivs vgonkivs requested a review from Wondertan September 18, 2023 10:38
@renaynay
Copy link
Member

I like nodebuilder/mod/cmds and +1 to @Wondertan 's comment otherwise it will be too messy when we try to take a look at it later on down the line.

@vgonkivs vgonkivs requested a review from ramin September 18, 2023 14:56
@vgonkivs vgonkivs marked this pull request as ready for review September 19, 2023 11:36
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

i've made a few basic style and naming comments i think it'd be nice to make but aren't essential. I wasn't expecting the nodebuilder//cmd as the refactor but really like it. I'd be fine if you ignore my name and organization things and merge, but think my ideas are great obviously 🤣

Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

utACK, but looks really good, no nits

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

  • let's call all subdirectories cmd instead of cmds - more idiomatic

@Wondertan Wondertan changed the title improvement(cmd): extract rpc cmds refactor(cmd): extract rpc cmds Sep 21, 2023
@vgonkivs vgonkivs merged commit 26642ad into celestiaorg:rpc_cmds_feature_branch Sep 25, 2023
vgonkivs added a commit to vgonkivs/celestia-node that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants