small units enhancements

[jessfraz]

• onboarding should prompt you for your app wide units in the first step
• bring back the warning if files has no @settings annotation

[nrc]

The onboarding thing seems straight forwardly good. I've kind of lost track of the discussion around defaults of various kinds and warnings and migration plans etc. So I think it would be good to write out the plan in one place.

So just to be clear, the various ways to get the units for a number are:

• explicitly, e.g., 42mm
• by default e.g., writing 42 and the units get computed from the most specific default and how the value is used (the mechanics of which deserve a proper design doc, which I've been meaning to write for a while, but haven't yet, sorry). There is both a default for lengths and for angles and they don't have to be specified in the same place. The possible sources of defaults are:
  • local-scope, using @settings(...) (not currently supported)
  • file-scope (technically module scope, but files and modules are currently the same thing), using @settings(...)
  • project-scope, using project.toml (currently only supports a default for lengths)
  • the default-default, currently mm and degrees

Of those defaults, I expect that we:

• Don't support local scope now or in the future
• Keep file-scope as the preferred mechanism used in our examples, docs, etc.
• Require a default-default because we need something in case there is no file default (technically we could just hard error and not render anything but that seems pretty aggressive).

And I'm not sure about:

• Are we keeping the project-scoped default length or removing this? And if removing, then when?
• How aggressively are we encouraging users to use a file-specific default vs relying on implicit defaults (either project-scope or default-default)? E.g., do we want errors, warnings, to automatically insert @settings on file creation, etc.?

And to reiterate the pros and cons as I understand them:

• Encouraging an explicit default makes the units more explicit and therefore less likely to have units-related footguns
• Short-term, we don't have support for libraries therefore there are likely to be 'library' modules in a project with different units to the rest of the project, so being explicit is good. However, long-term with libraries, it's likely that the common case is that a whole project uses the same units (or the same default units in every module, at least).
• If a user is mixing units, it is more reliable for them to use explicit units rather than rely on different defaults. Unfortunately we don't have enough experience with this style of programming to draw firm conclusions or to iterate on the design to make it more ergonomic.
• And related, but more philosophically, if we want to encourage being explicit about units, I think (though I'm unsure what this would look like) that it is better to encourage more usage of explicit units rather than being explicit about the defaults.
• Having a warning or error on missing defaults is an ergonomic road bump when writing code - since it doesn't change anything, the user will feel like they're just writing boilerplate to keep the interpreter happy and this is an extremely itchy experience
• The settings attribute is at the top of the file, a little syntax-heavy, and has pretty complicated semantics (what code does it affect? What is does default mean? How exactly do defaults get used? How do units work? Plus it's inherently non-local). So for new users reading a KCL file for the first time it is a big speed bump between 'opening a KCL file for the first time' and figuring out how code turns into a picture. And this is at the most vulnerable part of the user acquisition funnel.

[jtran]

#5808 already got rid of project-scoped. This is strongly desired because we want to be able to share a KCL file and have it contain everything needed to execute it, and not have the side-effect of mutating the current project, which was both error-prone and confusing. That PR ripped out units from the interpreter's project settings struct. The interpreter only knows about @settings(). There's still a project-level setting in the UI, persisted to project.toml. But its only use is writing the initial @settings() in new KCL files. There's also a user-level TOML config for the same purpose.

[nrc]

So, to para-phrase, we do have a project-wide default, but that is only used to generate per-file defaults rather than for selecting the units for a particular number.

My observation is that although the rationale as explained is solid, it does mean that not only do we have three different default units, we now have two different kinds of default units.

How does an app-wide setting factor into this? Will the per-project setting be moved to app-wide or will there be an app-wide setting to populate the per-project setting for new projects? (My preference would be to drop the per-project setting and use the app-wide setting to populate the @settings for new files).

and not have the side-effect of mutating the current project

I'm not sure I understand this point. I understand that moving a file to a different project would mean the contents of the file might change, but not how that would have an effect on the rest of the project?

[jtran]

WRT mutating... In the past, before #5808, when we loaded kcl-samples in the app, we had to overwrite the current project's settings to ensure that the sample displayed correctly. This would potentially break other files in the project. So we had to prompt users to confirm that they wanted to do a dangerous thing. It was confusing.

[jtran]

The app-wide setting is what I described as user-level. In the app, if you go to settings (Cmd+, or through the top-right menu), there are two tabs: User and This Project. User-level settings are stored in a settings.toml file. Project-level settings are stored in a project.toml in the project directory. Many but not all settings are present in both user and project. It's hierarchical in the sense that for a given config, it's looked up in the most specific level, and if not found, looks up the hierarchy. What we've been calling default-default length is what's used if it's not found at any level.

So currently, when a user creates a new KCL file in the app, it first looks at the project-level setting, then the user level setting, then falls back to the default-default which we've all agreed and doubled-down on mm. Instead of a blank KCL file, that unit is written to the file in @settings(). Currently, there's a check that if it equals the default-default, skip writing it. But presumably, if we change to warn when @settings() isn't present, we'll skip that check and always write it to new KCL files.

From then on, neither the user-level nor project-level will affect this file because they're only used to generate new files. The interpreter never sees these; it only ever sees @settings().

[jessfraz]

From then on, neither the user-level nor project-level will affect this file because they're only used to generate new files. The interpreter never sees these; it only ever sees @settings().

THIS 100% THIS.

[jtran]

@nrc, the latest Slack thread on the topic is what triggered this issue. I can try to summarize my understanding of it.

Although in principle I agree with you about @settings() at the top of the file not being nice for users, the thread's take-away seems to be that there's no universal right answer to what default the units should be. Even for people who would choose one unit, it really depends on the project what units they will use.

(And even though this may be an uncommon use case, I think we need to support projects where it's an assembly built out of parts/files that don't all use the same units. People should be able to make a part, and then when someone else comes along, they can use the part anywhere else, and it works. I.e. parts/modules are composable. It shouldn't matter how it was made.)

Another take-away from the discussion is that the default-default is a last resort for us implementing it. We all agree that it should match the engine API and be mm. We as a company are default-mm.¹ However, no users should ever hit this path in normal usage. They should have been explicit because the app's onboarding should have asked them what they want. Hence the desire for the warning when no @settings() is present.

In the discussion, @nrc, you explained how functions can be agnostic, using whatever is passed in as parameters. I've also had to explain this to people several times. I think a lot of people still don't get this. Maybe making an example and putting it somewhere visible, like in the docs, would be helpful. I've made examples of this before, but it was only ever shared privately. I can try to find this if you ping me. Probably needs to be updated anyway now that #6343 is merged.

Footnotes

1. Using SI units was brought up, but that would mean m, not mm. Our file import originally defaulted to m, and people complained, and we switched it to mm. ↩