Skip to content
This repository has been archived by the owner on Jan 27, 2025. It is now read-only.

Don't restrict bind scope to the bound var #83

Merged
merged 4 commits into from
Dec 3, 2021
Merged

Don't restrict bind scope to the bound var #83

merged 4 commits into from
Dec 3, 2021

Conversation

fsteeg
Copy link
Member

@fsteeg fsteeg commented Dec 1, 2021

Will resolve #66

@blackwinter
Copy link
Member

blackwinter commented Dec 1, 2021

I'm in the process of introducing virtual (internal) fields (for _id, see #63). Those could be leveraged here as well (add var as virtual field). I'll hold off with my PR (#84) until this one is merged.

@blackwinter
Copy link
Member

Catmandu only does this when the var option is given (ref.). OTOH, there's no test for a list bind without var right now.

Options:

  1. Treat list bind without var as undefined behaviour - subject to change (without notice).
  2. Add support for both cases (including tests).
  3. Disallow list bind without var for the time being (including tests).
  4. Officially support current behaviour (including tests) - intentionally diverging from Catmandu.

My preference would be 2.

@fsteeg
Copy link
Member Author

fsteeg commented Dec 2, 2021

Catmandu only does this when the var option is given

I don't get that. What's the point in not making the record available, in particular if we bind nothing to a var? Could that be a bug in Catmandu?

In general, I'd like to focus on getting our use cases working, and not worry too much about Catmandu compatibility at this point, therefore I'd vote for 1:

Treat list bind without var as undefined behaviour - subject to change (without notice)

@blackwinter
Copy link
Member

What's the point in not making the record available,

That was the initial implementation, so there must have been some point to it ;)

in particular if we bind nothing to a var?

Then your scope is determined by the path, you just can't access the whole record. If you need that, bind to a variable.

Could that be a bug in Catmandu?

As I said, I don't know if it's documented anywhere. But it was introduced as a feature enhancement (LibreCat/Catmandu@88dd6d1), the restricted scope was the original behaviour, AFAICS.

@fsteeg
Copy link
Member Author

fsteeg commented Dec 3, 2021

Oh, that was fast. Here's what I wrote but waited to send after the build was fixed:

I've added access to the bound values to understand Catmandu's behavior in hbz/Catmandu@093c6cb and added corresponding tests and implementation in 1b1ee8e.

I'll commit further tweaks based on your comments.

@fsteeg
Copy link
Member Author

fsteeg commented Dec 3, 2021

Thanks for the quick review, I think your points should be addressed in a70a7b8.

I will merge once all conversations are resolved.

@blackwinter
Copy link
Member

Great, thanks! I think everything's resolved now.

@blackwinter blackwinter assigned fsteeg and unassigned blackwinter Dec 3, 2021
@fsteeg fsteeg merged commit 020c94f into master Dec 3, 2021
@fsteeg fsteeg deleted the 66-bindScope branch December 3, 2021 16:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the full record available in binds
2 participants