Read configuration once only

[shivaraj-bh]

Before

⌨️  Preparing to develop project: .
❄️  nix --version️
❄️  nix --extra-experimental-features nix-command show-config --json️
❄️  nix eval '.#om.develop' --json️
❄️  nix eval '.#om.health' --json️
✅ Nix environment is healthy.

🍾 Welcome to the omnix project

After

❯ just run develop
❄️  nix --version️
❄️  nix --extra-experimental-features nix-command show-config --json️
❄️  nix eval '.#om' --json️
⌨️  Preparing to develop project: .
✅ Nix environment is healthy.

🍾 Welcome to the omnix project

Notice that we only evaluate the om config once in: nix eval '.#om' --json️

[srid]

omnix-common now exports two (instead of one) public functions. Why do we need two rather one? If we do need two, when should we use one over the other? What is the difrerence between them? I also ask, because the code comment does not clarify this for me.

[shivaraj-bh]

omnix-common now exports two (instead of one) public functions. Why do we need two rather one? If we do need two, when should we use one over the other? What is the difrerence between them? I also ask, because the code comment does not clarify this for me.

get_sub_configs is for cases like om.templates. It is also used in om.develop, but I guess we could use get_referenced_for there. And get_referenced_for is when it makes sense for user to pick one amongst many possible sub-configs.

[srid]

Do I understand correctly that one is same as other, except for returning multiple (instead of one) keys?

In that case, shouldn't get_referenced_for be defined in terms of (using) get_sub_configs since the latter is a superset of the former?

[shivaraj-bh]

Yes, but there is also that get_referenced_for requires the type T, that it is converting to, have a Default defined, but that is not the case in get_sub_config.

[srid]

I mean, why can't get_referenced_for be simply defined as (roughly) get_sub_configs().get("default").unwrap_or_default() instead of the 28 line function body that it presently is?

[srid]

Ah, the confusion is to do with reference field complect'ing with these functions. Hmm, need to rethink the types here ...

[srid]

Basically, I think pub config: BTreeMap<String, BTreeMap<String, serde_json::Value>>, should be newtype'd as separate struct, with its own impl methods that operate just on the BTreeMap without regard to other fields (like reference).

Decoupled from OmConfig, basically, so these two types and their operations can be reasoned about independently.

[srid]

I've tested this locally and it works. And I'm glad that this simplifies the code by consolidating config in one place, and getting rid of qualified_attr.rs (it really was complicated solution to begin with!).

Could you do the decoupling mentioned in comments? Then it can be merged.

[shivaraj-bh]

I've tested this locally and it works. And I'm glad that this simplifies the code by consolidating config in one place, and getting rid of qualified_attr.rs (it really was complicated solution to begin with!).

Could you do the decoupling mentioned in comments? Then it can be merged.

I did the decoupling, although not super happy with the name Config for pub config: BTreeMap<String, BTreeMap<String, serde_json::Value>> , but couldn’t think of anything better.

[srid]

This changed the semantics. flake_url should be without attr.

I'll try to fix this, but I do find get_sub_config_under to be rather hard to understand (hence less confident of making these changes). I guess the low-levelness of Rust is to blame in large part, forcing us to spend extra effort trying to keep things simple.