Clean up codebase

[SaadiSave]

trafficstars

[Enhancement]

Would you be open to cleaning up this codebase and making it more idiomatic? I can submit a PR to help.

[darleybarreto]

I think it would be great. Perhaps you could make a single PR, but making independent commits so it would be better to review (one by one).

[SaadiSave]

Okay, I'll begin as soon as possible. Will you be committing to main or merging any PRs soon? I'd like to reduce merge conflicts, so I'll start with the latest possible commit.

[simeonschaub]

I'd like to merge #197 first before we start refactoring parts of the code base. @davidanthoff would you have time next week to take a quick look at that together?

[davidanthoff]

In general I think this is great, but I would hold off for a while. We have Juliacon coming up, we still have pending PRs that I want to merge plus a list of a couple of very high priority things I'd like to get in before Juliacon, and that will certainly take up all the bandwidth I have in July.

Also, could you describe a bit more what you have in mind? In general, very small and targeted PRs would work best.

[SaadiSave]

Firstly, but I think that the way modules are structured is rather unidiomatic and makes the code hard to read. For example:

use juliaup::{
    command_selfchannel::run_command_selfchannel,
    command_selfuninstall::run_command_selfuninstall,
    command_config_backgroundselfupdate::run_command_config_backgroundselfupdate,
    command_config_startupselfupdate::run_command_config_startupselfupdate,
    command_config_modifypath::run_command_config_modifypath,
};

The code is not fully utilising the module system, and this looks like the naming conventions used in C, which has header hell and no module system, so you have to prefix everything. That's not needed in Rust.

So, this import could be simplified to:

use juliaup::command::{
    self_channel,
    self_uninstall,
    config::{
        background_self_update,
        startup_self_update,
        modify_path,
    },
};

You can immediately perceive the difference in readability.

And then, while using it:

// example
self_channel::run();

Second, the code is not formatted properly, with snippets like this:

fn get_juliaup_home_path() -> Result<PathBuf> {
    let entry_sep = if std::env::consts::OS == "windows" {';'} else {':'};

    let path = match std::env::var("JULIA_DEPOT_PATH") {
        Ok(val) => {
            let path = PathBuf::from(val.to_string().split(entry_sep).next().unwrap()); // We can unwrap here because even when we split an empty string we should get a first element.

            if !path.is_absolute() {
                bail!("The `JULIA_DEPOT_PATH` environment variable contains a value that resolves to an an invalid path `{}`.", path.display());
            };

            path.join("juliaup")
        }
        Err(_) => {
    let path = dirs::home_dir()
        .ok_or(anyhow!(
            "Could not determine the path of the user home directory."
        ))?
        .join(".julia")
        .join("juliaup");

            if !path.is_absolute() {
                bail!(
                    "The system returned an invalid home directory path `{}`.",
                    path.display()
                );
            };

            path
        }
    };

    Ok(path)
}

This is the simplest to fix: a simple cargo fmt would do the trick. Plus, formatting checks should probably be added to CI.

Next, and this is rather subjective, but commands should probably be structs containing all that a command needs, and then a simple trait to run it:

pub trait Command {
    type Error;

    fn run(self) -> anyhow::Result<Self::Error>;
}

Which would be used like so:

use juliaup::command::{self, Command};

fn any() {
    command::Add::new(/* params */).run();
}

Lastly, linting with cargo clippy would also be rather helpful in catching unidiomatic code (use it in CI too).

I understand that these are massive changes, so take your time to debate them. I believe that these changes will make the codebase far more approachable.

[SaadiSave]

Possible refactored module tree:

As text:

crate juliaup_skeleton
├── mod command: pub
│   ├── struct Add: pub
│   ├── struct Api: pub
│   ├── trait Command: pub
│   ├── struct Default: pub
│   ├── struct Gc: pub
│   ├── struct InitialSetup: pub
│   ├── struct Link: pub
│   ├── struct List: pub
│   ├── struct Remove: pub
│   ├── struct SelfChannel: pub
│   ├── struct SelfUninstall: pub
│   ├── struct SelfUpdate: pub
│   ├── struct Status: pub
│   ├── struct Update: pub
│   └── mod config: pub
│       ├── struct BackgroundSelfUpdate: pub
│       ├── struct ModifyPath: pub
│       ├── struct StartupSelfUpdate: pub
│       └── struct Symlinks: pub
├── mod global_paths: pub
│   ├── struct GlobalPaths: pub
│   └── fn get_paths: pub
├── mod utils: pub
│   ├── fn get_arch: pub
│   ├── fn get_bin_dir: pub
│   ├── fn get_juliaserver_base_url: pub
│   └── fn parse_versionstring: pub
└── mod version_db: pub
    ├── struct Channel: pub
    ├── struct Version: pub
    ├── struct VersionDB: pub
    └── fn load: pub

[SaadiSave]

@davidanthoff, what is your opinion on this proposal?

[davidanthoff]

I need to think a bit about it, right now I'm too busy getting things ready for Juliacon. I'll engage once Juliacon is over.

[SaadiSave]

Okay. Meanwhile, I will begin planning the PRs.

[uncomfyhalomacro]

whats the state of this currently @SaadiSave and @davidanthoff ?

[SaadiSave]

@uncomfyhalomacro I have finished planning the PRs.

• PR 1: Reformat and lint
  1. Run cargo fmt
  2. Lint with clippy
  3. Add both to CI
• PR 2: Refactor API
• PR 3: Add tests?

[SaadiSave]

@davidanthoff do I have the green light?

[uncomfyhalomacro]

@SaadiSave I guess open the PRs. They usually are lost from multiple issues hanging around.

[SaadiSave]

@SaadiSave I guess open the PRs. They usually are lost from multiple issues hanging around.

Okay, I shall open the first PR shortly

[uncomfyhalomacro]

sorry for the sudden bump @davidanthoff. can @SaadiSave contribute on this one? he was very excited to do this.

[SaadiSave]

@uncomfyhalomacro thanks for the encouragement, but I currently have some personal problems to deal with. And uni exams are coming. I won't be able to contribute until March. I still am just as excited, but I simply don't have the time right now. Nevertheless, I shall try my best to get in a few small PRs by the end of this year.

[davidanthoff]

My view on this is that generally we should do this, but probably better to do it at a point where things calm down in terms of churn. Right now I'm trying to get everything finished for a release that we can call the official installer for Julia, and so there is a fair bit happening on the code.

[uncomfyhalomacro]

okay. we are happy to contribute and help when that time comes. :heart:

[SaadiSave]

My view on this is that generally we should do this, but probably better to do it at a point where things calm down in terms of churn. Right now I'm trying to get everything finished for a release that we can call the official installer for Julia, and so there is a fair bit happening on the code.

Is there perhaps a checklist of things left to do before the installer is considered to be MVP for an official installer?

[uncomfyhalomacro]

My view on this is that generally we should do this, but probably better to do it at a point where things calm down in terms of churn. Right now I'm trying to get everything finished for a release that we can call the official installer for Julia, and so there is a fair bit happening on the code.

Is there perhaps a checklist of things left to do before the installer is considered to be MVP for an official installer?

Same question. bump

[uncomfyhalomacro]

Hello. @davidanthoff I am planning to start contributing to this by moving around some things. But I also need your approval for doing this PR as JuliaCon is coming close now and I know you will be busy. Would it be okay to see a checklist of sorts so I can open small PRs so that it won't be giving you a large diff and so others can review.

[SaadiSave]

@uncomfyhalomacro what kind of improvements do you have in mind that could be done incrementally? The original proposal (https://github.com/JuliaLang/juliaup/issues/313#issuecomment-1181725350) involved restructuring the entire module tree, and I don't see how it could be done incrementally.

[uncomfyhalomacro]

@uncomfyhalomacro what kind of improvements do you have in mind that could be done incrementally? The original proposal (#313 (comment)) involved restructuring the entire module tree, and I don't see how it could be done incrementally.

I haven't thought of that yet. If we follow that proposal, yeah it would be a big PR. It seems I intend to follow your proposal too. So no small PRs then. 🤧

[SaadiSave]

I haven't thought of that yet. If we follow that proposal, yeah it would be a big PR. It seems I intend to follow your proposal too. So no small PRs then. 🤧

@uncomfyhalomacro perhaps we could start by running pedantic clippy on one module at a time? It will be much less ambitious than the original plan, but it would still appreciably improve code quality.

[uncomfyhalomacro]

Sure. I will do that during my weekends.

[davidanthoff]

While I'm still in favor of this in general eventually, can we hold off for now? This will just create extra work at the moment, and I'm trying to get some important features/bug fixes in that I think have a much higher priority.

[SaadiSave]

trying to get some important features/bug fixes in that I think have a much higher priority.

Is there a priority list? Perhaps we can help. I tried checking the issues but there are no labels for prioritisation.

[davidanthoff]

Probably this https://github.com/JuliaLang/juliaup/milestone/7.

[uncomfyhalomacro]

Sure. I will try to help with providing features or bugfixes before this then. 😄

[SaadiSave]

@davidanthoff

I believe that reworking the architecture of juliaup would fix many of the pending issues. Since juliaup is the official installer for julia now (at least, it appears to be recommended by default at https://julialang.org/downloads/), should we begin?