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: Move code from main.go to other files #164

Merged
merged 4 commits into from
Nov 6, 2024
Merged

Conversation

jirihnidek
Copy link
Contributor

@jirihnidek jirihnidek commented Nov 5, 2024

  • Card ID: CCT-950
  • Initial step of refactoring main.go.
  • Code moved from main.go to other files
  • Introduced new files for connect and disconnect commands
  • Added several other files for interactive mode, logging
  • Some code was moved to existing files
  • Unused code was removed.

Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

I have a concern with the change caused by this PR. While I see the benefit to reorganizing the files, this PR will create conflicts that need to be resolved in other PRs when they rebase onto these changes. This project is under fairly active development by a few different authors, and I would like to avoid putting unnecessary conflict resolution into those other PRs, if possible. Could we be more methodical about the refactor? Perhaps we could move the functionality of subcommands one commit at a time? Or first mark these "action" functions as deprecated, build new subcommand data structures that call back into these original "action" functions and then remove them?

I do something similar to this in xrhidgen. Each subcommand is declared as a global variable (for instance, associateCommand). Because that variable is a Command instance, it can be listed directly in the top level list of subcommands (as seen here in main.go).

Perhaps we can do something like that? There is no scenario I can think of that won't force some sort of conflict resolution upon the other outstanding PRs, but I'd like to make sure we do what we can to reduce the amount of conflict resolution we put upon the other PRs.

util.go Show resolved Hide resolved
@jirihnidek
Copy link
Contributor Author

I have a concern with the change caused by this PR. While I see the benefit to reorganizing the files, this PR will create conflicts that need to be resolved in other PRs when they rebase onto these changes. This project is under fairly active development by a few different authors, and I would like to avoid putting unnecessary conflict resolution into those other PRs, if possible. Could we be more methodical about the refactor? Perhaps we could move the functionality of subcommands one commit at a time? Or first mark these "action" functions as deprecated, build new subcommand data structures that call back into these original "action" functions and then remove them?

There is no open PR that could IMHO have conflict with this PR, because other opened PRs are related to build system & rhc.spec file or integration tests. Only my PR #46 could have some conflicts, but it will be easy to resolve. I think that this is great time to do this refactoring in one PR.

@subpop
Copy link
Collaborator

subpop commented Nov 6, 2024

There is no open PR that could IMHO have conflict with this PR, because other opened PRs are related to build system & rhc.spec file or integration tests. Only my PR #46 could have some conflicts, but it will be easy to resolve. I think that this is great time to do this refactoring in one PR.

I see where you're coming from. It is a good time to reorganize things into files. And no matter what, we'll have to eventually convert these action functions into something else. I just want to make sure we're being conscious of what impact a reorganization like this will have on other committers. I think if we just separate the removal of unused functions into a separate commit, this will be fine to merge.

* Card ID: CCT-950
* Initial step of refactoring main.go.
* Code moved from main.go to other files
* Introduced new files for connect and disconnect
  commands
* Added several other files for interactive mode,
  logging
* Some code was moved to existing files
* Renamed register.go to rhsm.go, because it contains code
  not only for registration
* This function is not needed anymore. This function has not been
  used since #149 was merged
* This function has not been used since PR #149 was merged to
  main branch
* No need to use go-ini module
* This function was used only in GuessAPIURL() and it was removed
  in previous commit: f7e8ffa
@jirihnidek
Copy link
Contributor Author

@subpop @m-horky I updated the PR as requested.

@subpop subpop merged commit 55f1ef2 into main Nov 6, 2024
20 of 22 checks passed
@subpop subpop deleted the jhnidek/refactor_main branch November 6, 2024 19:01
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