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

Extend Maps support. #85

Closed
blackwinter opened this issue Dec 6, 2021 · 8 comments · Fixed by #86
Closed

Extend Maps support. #85

blackwinter opened this issue Dec 6, 2021 · 8 comments · Fixed by #86
Assignees

Comments

@blackwinter
Copy link
Member

blackwinter commented Dec 6, 2021

In order to allow lookups to access separately defined maps, I'd like to propose the following changes:

  • Let Metafix implement Maps interface to allow adding maps programmatically (including internal Java maps¹).
  • Introduce new (custom) Fix functions (names up for discussion):
    • put_map("mapName", "key": "value", ...): Define internal map inside Fix script.
    • put_filemap("fileName"[, "mapName"][, "sep_char": "..."]): Define external file map.
  • Update existing lookup function usages:
    • lookup("fieldName", "key": "value", ...): Access local map (unchanged).
    • lookup("fieldName", "fileName"[, "sep_char": "..."]): Access external file map (see above); will be defined on first use (unless already defined via put_filemap) and reused afterwards (ignoring any options).
    • lookup("fieldName", "mapName"): Access internal map (see above); must be defined (via put_map), otherwise interpreted as new file map.

¹ No support for standalone Java maps (put_javamap?) required yet.

@blackwinter blackwinter self-assigned this Dec 6, 2021
blackwinter added a commit that referenced this issue Dec 6, 2021
blackwinter added a commit that referenced this issue Dec 6, 2021
@blackwinter
Copy link
Member Author

Functional review: @TobiasNx
Code review: @fsteeg

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Dec 7, 2021
tibdevelopment pushed a commit to TIBHannover/oersi-etl that referenced this issue Dec 8, 2021
@TobiasNx
Copy link
Collaborator

TobiasNx commented Dec 8, 2021

put_mapseems to work
in one line and with linebreaks:

put_map("map",
"dog":"mammal",
"cat":"mammal",
"parrot":"bird",
"shark":"fish",
"dragon":"ficational animal",
"unicorn":"ficational animal"
	)

put_filemap works too, I was a little bit confused that the path is first an the mapName is second which is "differet" to put_map where the first variable is the first.

Also the reuse of the options in lookup seems to work.

See for tests:
https://gitlab.com/oersi/oersi-etl/-/tree/FunctionalReview/data/experimental/map

+1

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Dec 8, 2021
@blackwinter
Copy link
Member Author

Thanks for the review!

put_filemap works too, I was a little bit confused that the path is first an the mapName is second which is "differet" to put_map where the first variable is the [mapName].

That's because lookup (in Catmandu) works on the file name, so this is the primary means of addressing external file maps. Naming an external file map is optional, hence the second parameter.

@TobiasNx
Copy link
Collaborator

TobiasNx commented Dec 8, 2021

Oh, one side note:

Not all attributes have (at least optional) specifications (what is the specific term idk?) what they are. Only some have as "sep_char":.

I found this a little bit annoying/confusing with catmandu and not user friendly specially with the many attributes/options,
And reviewing your ticket I thought about it again.

Something like this would be nice:
lookup("field":"[fieldName], "in":"[fileName], "sep_char": ";")

Even nice if the options/attributes would not have quotationmarks. This also touches: #74 because if we had consistently
lookup(field:"[fieldName], in:"[fileName], sep_char: ";")

@blackwinter
Copy link
Member Author

blackwinter commented Dec 8, 2021

Not all attributes have (at least optional) specifications (what is the specific term idk?) what they are.

There are "parameters" and "options", if that's what you mean. Parameters are unnamed, options are named.

The problem with maps in particular is that we (ab)use the options as local map, so we can't have actual options as well.

Even nice if the options/attributes would not have quotationmarks.

You don't need them here, as long as your keys/values only contain word characters¹:

 lookup(field: fieldName, in: fileName, sep_char: ";")

¹ EDIT: And as long as they're not reserved keywords.

@TobiasNx
Copy link
Collaborator

TobiasNx commented Dec 8, 2021

The problem with maps in particular is that we (ab)use the options as local map, so we can't have actual options as well.

Even nice if the options/attributes would not have quotationmarks.

You don't need them here, as long as your keys/values only contain word characters¹:

 lookup(field: fieldName, in: fileName, sep_char: ";")

¹ EDIT: And as long as they're not reserved keywords.

But that means that having corresponding parameters/options is not easy/possible?
It makes the fix more readable, it is what i liked about the morph.

@blackwinter
Copy link
Member Author

But that means that having corresponding parameters/options is not easy/possible?

No, I don't think that's possible in general.

@fsteeg
Copy link
Member

fsteeg commented Dec 14, 2021

For the record, if I understand the discussion correctly, the suggestion by @TobiasNx to always use named parameters is basically what I had in mind writing this comment:

// TODO SPEC: switch to morph-style named params in general?

Right now we have unnamed and named parameters since it's what Catmandu does. In a future attempt to improve the language while working on a common spec (hence TODO SPEC above) it might make sense to consider the approach from Morph, where we have named paramters only.

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

Successfully merging a pull request may close this issue.

3 participants