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

cue: improve documentation for Value.UnifyAccept #2300

Open
kortschak opened this issue Mar 18, 2023 · 16 comments
Open

cue: improve documentation for Value.UnifyAccept #2300

kortschak opened this issue Mar 18, 2023 · 16 comments
Assignees
Milestone

Comments

@kortschak
Copy link

kortschak commented Mar 18, 2023

What version of CUE are you using (cue version)?

Not using cue as an executable but rather via the Go API.

$ grep cuelang go.mod                        
	cuelang.org/go v0.6.0-0.dev

Does this issue reproduce with the latest stable release?

Yes

What did you do?

Using the Go API I would like to be able to unify a set of values while ignoring some fields that are only transiently generated but useful to retain for logging purposes.

For example if I have

{
    sum: "a31c34da"
    important1: "value1"
    config: {
        part1: "value2"
    }
} unify ignoring sum {
    sum: "ae20a82c"
    important2: "value3"
}

I would like to be able to resolve this to something like

{
    sum: "a31c34da"|"ae20a82c"
    config: {
        part1: "value2"
    }
    important1: "value1"
    important2: "value3"
}

or similar with the sum field omitted.

It looks to me from the documentation that something like this is supported by cue.Value.UnifyAccept, but how to use it to do this is unclear and I have not been able to get it to work in my experiments. Can the documentation be clarified for this?

What did you expect to see?

Clear explanation of how to use UnifyAccept.

What did you see instead?

Limited explanation. I was able to find one use of it in the tree and no tests, so it is difficult to know how to use it, or even whether my interpretation that it is usable for this is correct.

@kortschak kortschak added NeedsInvestigation Triage Requires triage/attention labels Mar 18, 2023
@myitcv myitcv added Documentation and removed NeedsInvestigation Triage Requires triage/attention labels Apr 6, 2023
@myitcv
Copy link
Member

myitcv commented Apr 6, 2023

I'm not altogether certain there isn't a bug lurking here given that method doesn't have tests.

@myitcv
Copy link
Member

myitcv commented Apr 6, 2023

Oops, that sent early. Thanks for the report, @kortschak.

@myitcv
Copy link
Member

myitcv commented Jun 13, 2023

Noting that #454 is, I think, effectively the same as what you would like to achieve in this case.

That said, the UnifyAccept docs appear (per @mpvl) to be wrong. They should be improved, and a test added. Which we will aim to do in v0.6.0.

@myitcv myitcv added this to the v0.6.0 required fields milestone Jun 13, 2023
@kortschak
Copy link
Author

Thanks. I'm not sure that #454 does exactly what I was looking for since it requires that fields be missing as opposed to the situation I have where I'd like explicitly not missing and disagreeing fields to be ignored, unless I'm missing something in that proposal for how to do that.

@mpvl mpvl self-assigned this Jun 28, 2023
cueckoo pushed a commit that referenced this issue Jun 30, 2023
Issue #2300

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: If00caaad07ccd7519aeb7f2a8bdeb187a5854d93
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/556193
TryBot-Result: CUEcueckoo <cueckoo+gerrithub@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Paul Jolly <paul@myitcv.io>
@mpvl mpvl assigned myitcv and mvdan and unassigned mpvl Jun 30, 2023
@mvdan
Copy link
Member

mvdan commented Jul 4, 2023

@kortschak with a downcast, you can take a CUE struct and remove any fields which are not in another struct. Roger added an example in #454 (comment) which I find useful.

Going from the example you shared at the top, here are my thoughts on your scenario: https://tip.cuelang.org/play/?id=qFlvpZpJztc#cue@export@cue

If you want to simply omit the field sum, then the second approach works for you today, and I don't think downcasting would help that much. You can use a similar mechanism to omit a set of fields only, including pattern matching.

If you want to instead omit everything except a set of fields (what you could call an "allowlist" filter instead of a "blocklist"), then I think the third approach with downcasting would be better. It really depends on whether your data fields are a well-defined set or not.

@mvdan
Copy link
Member

mvdan commented Jul 4, 2023

I'm going to remove this issue from the v0.6.0 milestone, since we want to get that out very soon, and we're focusing on siginficant regressions and bugs, such as panics or bad output. I'm more than happy to help unblock you regardless, of course :)

@mvdan mvdan modified the milestones: v0.6.0 required fields, v0.6.x Jul 4, 2023
@mvdan mvdan unassigned myitcv Jul 4, 2023
@kortschak
Copy link
Author

Thanks @mvdan. I'm not blocked by this — I am working around it — but yes, the #MinusSum approach looks like what I want.

@myitcv
Copy link
Member

myitcv commented Jul 4, 2023

@kortschak in your original description, you have two values (which I will refer to as a and b):

a: {
	sum:        "a31c34da"
	important1: "value1"
	config: {
		part1: "value2"
	}
}

b: {
	sum:        "ae20a82c"
	important2: "value3"
}

Do you know the schema of a and b? Or is everything apart from the sum field unknown? I'm assuming you do know something about the schemas because your expectation is that the unification of a modulo sum and b modulo sum should succeed.

Linking back to my thinking on how/why downcast might be related, therefore consider:

https://tip.cuelang.org/play/?id=U2kBHrwp3KP#cue@export@cue

If, however, you are looking for something more "automatic" where nothing is known about a and b, then it might be (per @mpvl) that what you are after is something closer to anti-unification (#7, #15, #2231).

Are you able to share a bit more context on the "inputs" a and b here? And also where/how the result will be used?

@kortschak
Copy link
Author

@myitcv The types and the skeleton of the schema that I'm using are here https://go.dev/play/p/2nyCaScb9O7. There are a few other parts, but they are largely irrelevant to this issue. The Sum field in the Kernel, Module and Service structs is the field that can conflict. Conflicts in any other fields noted in the schema indicate an invalid configuration.

@myitcv
Copy link
Member

myitcv commented Jul 4, 2023

The Sum field in the Kernel, Module and Service structs is the field that can conflict. Conflicts in any other fields noted in the schema indicate an invalid configuration.

Thanks for sharing this. To further help my understanding, what does it mean to unify a Kernel, Module and Service? They seem like quite distinct things, and I can't say I would have guessed that it could/should be possible to unify them.

@kortschak
Copy link
Author

Oh, sorry, that is a crucial part that I missed out. The application takes configuration snippets (as TOML) and aggregates them in to a single configuration (a valid System), so the unifications are S₁, S₂, S₃ etc → S.

@myitcv
Copy link
Member

myitcv commented Jul 5, 2023

Ah I see now. In which case I would suggest @mvdan's approach of #MinusSum is a reasonable one in the absence of downcast: you are dealing with concrete data (only regular fields) and the conflict is known to only occur at the top level.

The reason downcast will, in general, be more useful is that the "removal" of a field might need to occur at a nested point, in which case specifying "recursive" comprehensions gets messy very fast.

@kortschak
Copy link
Author

Thanks. At the moment I'm just removing the field in Go before the validation and then replacing after. This is ugly, but simple. downcast looks similarly simple. (When) will it be added if ever?

@myitcv
Copy link
Member

myitcv commented Jul 11, 2023

downcast looks similarly simple. (When) will it be added if ever?

I've added a comment to #943 so that it is considered as part of that batch of builtins. We are planning to look through that list as part of the v0.7.0 changes, so will know more on timings/target release then.

@sdboyer
Copy link

sdboyer commented Jul 11, 2023

Just want to register a general interest in builtins in this area. I realize there may be nuanced differences, but when i look at cut, downcast, #MinusSum between here and #454, they all strike me as being in a similar vein to TypeScript's Omit: given a definition of a type-thing, make a new type-thing that lacks certain fields/keys.

This could be quite valuable for reducing copypasta in some of our user flows.

@myitcv
Copy link
Member

myitcv commented Jul 11, 2023

they all strike me as being in a similar vein to TypeScript's Omit:

With one important distinction to at least my understanding: downcast as proposed would be recursive in its implementation. As far as I can tell, Omit applies at the top level of Type (its first type parameter).

@sdboyer thanks for registering the interesting. I suggest we continue the conversation over in #943.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants