[Renaming] ComputeNixEnv -> ComputeEnv; ensurePackagesAreInstalled -> ensureStateIsCurrent

[savil]

Summary

These names were getting rather stale. Hopefully, the new ones represent
them more accurately.

How was it tested?

compiles and cicd tests should pass

[savil]

Current dependencies on/for this PR:

• main
  • PR [print-dev-env] Skip cache if lockfile has changed #1674
    • PR [Renaming] ComputeNixEnv -> ComputeEnv; ensurePackagesAreInstalled -> ensureStateIsCurrent #1675 👈

This stack of pull requests is managed by Graphite.

[mikeland73]

There's a bunch of other functions you will need to change if you want to keep consistency:

Good call.

• NixEnv -> DevboxEnvExports
• NixEnvOpts -> DevboxEnvExportsOpts
• ensurePackagesAreInstalledAndComputeEnv -> computeCurrentDevboxEnv
• NixEnvPath (maybe?) -> yep, devboxEnvPath

@mikeland73 PTAL

[savil]

Also, treating current as a shorter synonym for upToDate (shorter in terms of number of words) but maybe I should change to that?

[savil]

@mikeland73 PTAL

[savil]

ping

[mikeland73]

Some comments. Approving to unblock.

Only blocker for me:

computeDevboxEnv vs computeCurrentDevboxEnv

This distinction means nothing to me. computeCurrentDevboxEnv should be something like ensureProjectStateIsCurrentAndComputeEnv. If you want to shorten it could also be ensureStateIsCurrentAndComputeEnv

I think when a function is complex, giving it a longer name for clarity is better.

I also think in many cases we can drop Devbox from variables and function names. So computeEnv or computeEnvironment. At the end of the day, the struct is named devbox so it's fairly clear what the subject is.

[mikeland73]

EnvExports ? So if the struct name is devbox:

devbox.EnvExports ?

Also a bit repetitive but usually our option structs end in Opt. We should leave for consistency. (so EnvExportOpts)

[savil]

Yeah, lets call it EnvExports. Good call.

Yeah, I drop the Opt because devopt already specifies it. For consistency, I can drop it from the other places as well? Happy to do a follow up.

[savil]

Upon further inspection, I am renaming to EnvExportOpts and adding this naming guideline in the devopts package:

// Naming Convention:
// - suffix Opts for structs corresponding to a Devbox api function
// - omit suffix Opts for other structs that are composed into an Opts struct

[mikeland73]

make sure trace tasks and profiling names are changed too.

[savil]

Some comments. Approving to unblock.

Only blocker for me:

computeDevboxEnv vs computeCurrentDevboxEnv

This distinction means nothing to me. computeCurrentDevboxEnv should be something like ensureProjectStateIsCurrentAndComputeEnv. If you want to shorten it could also be ensureStateIsCurrentAndComputeEnv

I think when a function is complex, giving it a longer name for clarity is better.

I also think in many cases we can drop Devbox from variables and function names. So computeEnv or computeEnvironment. At the end of the day, the struct is named devbox so it's fairly clear what the subject is.

The distinction is: computeEnvThatWillBeUpToDate versus computeEnvThatMightBeStale.

I don't see the value in being so explicit TBH, because then we'll end up with super long sentences for functions.

ensureStateIsCurrentAndComputeEnv is not a good name because it focuses on the implementation and not the actual meaning goal. The goal is to get the environment that we can guarantee is up-to-date.

If you're not sure the first time you encounter the name, you can inspect the docblock of the function to understand what it does, and in your mind you'll realize "Ah, the current is referring to ensuring the environment is current or up to date". And henceforth it'll be clear.

[mikeland73]

The distinction is: computeEnvThatWillBeUpToDate versus computeEnvThatMightBeStale.

This is not quite why ensurePackagesAreInstalledAndComputeEnv was introduced (see below)

ensureStateIsCurrentAndComputeEnv is not a good name because it focuses on the implementation and not the actual meaning goal. The goal is to get the environment that we can guarantee is up-to-date.

My issues with computeCurrentDevboxEnv are:

• It's very similar to computeDevboxEnv.
• current is not a strong enough descriptor of what it is doing. current means "the current thing" and it also means "up to date". The former is probably more common.
• The function is not only computing the up to date environment, it is making the internal state be up to date. It changes the hidden files and installs nix packages. I'm also not sure I agree that making it up to date is not a goal (I would argue it definitely is). This function was introduced because there were 3-4 places where we repeated the exact same steps, but the goal is to do both, 1) bring the state up to date 2) compute the env.
• My view with naming is that when meaning in doubt, be explicit.

Does that make sense?

[savil]

The distinction is: computeEnvThatWillBeUpToDate versus computeEnvThatMightBeStale.

This is not quite why ensurePackagesAreInstalledAndComputeEnv was introduced (see below)

ensureStateIsCurrentAndComputeEnv is not a good name because it focuses on the implementation and not the actual meaning goal. The goal is to get the environment that we can guarantee is up-to-date.

My issues with computeCurrentDevboxEnv are:

• It's very similar to computeDevboxEnv.
• current is not a strong enough descriptor of what it is doing. current means "the current thing" and it also means "up to date". The former is probably more common.

I'd already indicated that I wasn't sure if current was best. I've switched to upToDate.

• The function is not only computing the up to date environment, it is making the internal state be up to date. It changes the hidden files and installs nix packages. I'm also not sure I agree that making it up to date is not a goal (I would argue it definitely is). This function was introduced because there were 3-4 places where we repeated the exact same steps, but the goal is to do both, 1) bring the state up to date 2) compute the env.
• My view with naming is that when meaning in doubt, be explicit.

Does that make sense?

Fair enough. And its not such a big deal to me so I've changed it for now.

But I have added a TODO to find a better name.

I consider it a mild code-smell to introduce a function when the callers can just directly invoke the core-primitives themselves (in this case, ensureStateIsUpToDate and computeEnv). By adding this function, we do get the small convenience of avoiding two function calls, but then add the cognitive overhead of needing to remember that "oh, we also have this small convenience function".

And then, over time, as has happened here with the "no such host" check, we introduce behavior changes as well. So, it is no longer exactly composing the two functions. And then this begs the question, what is the "core concept of this function" and when should one invoke it versus directly invoking the two functions it is composing?

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ **exec.ExitError: <redacted *errors.withStack>: <redacted errors.withMessage>: <redacted exec.ExitError> go.jetpack.io/devbox/internal/nix in (*Nix).Pri... View Issue
• ‼️ **exec.ExitError: <redacted usererr.combined>: <redacted fmt.wrapError>: error running "nix profile install": <re... go.jetpack.io/devbox/internal/nix/nixprofile in... View Issue
• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

_{Did you find this useful? React with a 👍 or 👎}