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 the entry point to make it more readable #1172

Merged
merged 12 commits into from
Jun 2, 2023

Conversation

ThomasFrans
Copy link
Contributor

@ThomasFrans ThomasFrans commented May 22, 2023

Quite some Rust TUI applications move their 'global' data into a struct called App or Application. Other types of Rust (or non-Rust) applications might do this as well, I just checked TUI Rust applications. This makes it very clear what data belongs to the 'global' scope or in other words, to the application itself. For ncspot, this would be things like the queue, the library, the web API, probably the parsed configuration, Cursive and probably some other things. It makes the main.rs file more readable and less scary looking for new contributors, and gives a nice overview of what ncspot consists out of from looking at the Application struct definition.

Programs I checked were:

  • zellij client
  • spotify-tui
  • wiki-tui
  • gitui
  • ox
  • helix
  • some others (I forgot)

I forgot which ones used this pattern exactly, but most of them used it in one way or another.

This is just a suggestion, in an attempt to clean up the code around the entry point a bit. The bigger idea, which I don't know whether it is a good idea or not, is to make the ownership of items a bit more clear. Personally I find it hard to get a grasp of 'who owns what'. I wanted to attempt to make this a bit clearer, and this would be a first step. This PR won't be finished for a bit, but I wanted to make it already to hear whether this idea was maybe not a good direction to go, in which case I will close it 🙂

Some applications move their 'global' data into a struct called `App` or
`Application`. This makes it very clear what data belongs to the
'global' scope or in other words, to the application itself.
To have a clear distinction between code dealing with OS process
characteristics and code of ncspot itself, it makes sense to move the
async runtime together with ncspot as it doesn't have anything to do
with the OS process.
Command line arguments are part of the OS process and should be under
main.
Sometimes the use of an item wasn't clear without reading the code. This
should help with that.
@ThomasFrans ThomasFrans changed the title refactor: move 'global' data into Application Refactor the entry point to make it more readable May 27, 2023
@ThomasFrans
Copy link
Contributor Author

This PR is just a suggestion with lots of small changes, in the hopes that it can make some of the code more readable. I don't mind if it is closed 🙂

As I don't want this to become a 1000 line diff, I'll mark this one as finished for now. There are some parts that I think could be adjusted further, but it would make this PR too big to review. I tried to clean up some parts of the code that I found a bit harder to read. I realize this isn't a 'do one thing' type of PR, as when I see something to improve, I can't help myself and try to improve it, leading to lots of different changes. If some smaller PRs are preferred, I'll happily split this one up.

  • Move most of the code from the main() function into an Application struct, like a lot of other TUI Rust applications. This makes the entry point more readable for new contributors, and shows what data 'belongs to the application' like the queue, the CursiveRunner, the IPC implementations...
  • Move all the functions from the main module into other places to clean up the entry point. The functions were moved into places where they made sense, like the credentials functions which were moved under authentication.
  • Add documentation to various items to clarify their purpose. (The Spotify struct got a TODO comment as I don't fully understand its purpose, so I couldn't document it properly.)
  • Pull some code that was previously in the main() function into separate functions. The idea is to prefer clearly named functions over adding comments in the code itself.
  • Create a create_cursive() function to create the CursiveRunner for both the main program as well as the authentication part.
  • Use PathBufValueParsers for parsing the path CLI arguments with Clap, as it makes clear that a proper path is expected and checks for a proper path during the parsing step.
  • Change function signatures to take ownership/borrow when needed, as it can be more efficient when no unneeded clones are needed.
  • Fully qualify paths in code locked behind a feature, to prevent unnecessary includes when the feature isn't enabled (fixes some Clippy warnings on Windows and macOS).
  • Remove some global Cursive callbacks in favor of reacting to events on a view itself (specifically for Layout). This cleans up the code inside application and more clearly shows where those events are used. I realized after this change that maybe the goal was to make Layout more generic. It is currently only used in one place and there will probably only be a single use case, so I felt like it made sense to apply this change. I'll revert it if needed 🙂

Somehow the final stripped release binary also shrunk 300kb, which wasn't a goal, but is still a good thing I guess 😄

@ThomasFrans ThomasFrans marked this pull request as ready for review May 28, 2023 09:40
@hrkfdn
Copy link
Owner

hrkfdn commented Jun 2, 2023

I really appreciate all the effort you put into this. I only had a brief look, but everything looks nice and manual testing didn't reveal any issues. I'd merge this now and if something comes up we can still fix it in tree.

❤️

@hrkfdn hrkfdn merged commit 96bb2ea into hrkfdn:main Jun 2, 2023
@ThomasFrans ThomasFrans deleted the entrypoint-cleanup-suggestion branch June 3, 2023 13:08
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.

None yet

2 participants