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

Optionally set key and/or value column. #471

Merged
merged 4 commits into from
Oct 17, 2022
Merged

Optionally set key and/or value column. #471

merged 4 commits into from
Oct 17, 2022

Conversation

blackwinter
Copy link
Member

@TobiasNx
Copy link
Contributor

Is this then also usable in metafacture-fix?

@blackwinter
Copy link
Member Author

Only after we add the corresponding options to put_filemap().

Copy link
Member

@dr0i dr0i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me!

@blackwinter blackwinter marked this pull request as ready for review October 13, 2022 16:09
@blackwinter
Copy link
Member Author

Seems good to me!

Thanks, but wasn't ready for review yet. Can you have another look?

@TobiasNx: Can you do the functional review?

@blackwinter blackwinter assigned dr0i and TobiasNx and unassigned blackwinter Oct 13, 2022
@TobiasNx
Copy link
Contributor

I will do an functional review.

Copy link
Member

@dr0i dr0i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update ./metamorph/src/main/resources/schemata/metamorph.xsd ? This is the scheme helping e.g. XML editors validating a Morph.

@blackwinter
Copy link
Member Author

Can you update ./metamorph/src/main/resources/schemata/metamorph.xsd ?

Will do, thanks.

This is the scheme helping e.g. XML editors validating a Morph.

Not only editors, Metamorph itself uses it. I just didn't notice since I had foregone the Morph tests.

@blackwinter blackwinter requested a review from dr0i October 14, 2022 09:12
@dr0i dr0i removed their assignment Oct 14, 2022
@TobiasNx
Copy link
Contributor

TobiasNx commented Oct 14, 2022

I am still not sure about where to add the options:

	<maps>
		<filemap name="animals" files="multipleColumnLookup/animals.tsv" separator="\t" expectedColumns="-1"/>
	</maps>
	<rules>
		<data source="animal">
			<lookup in="animals" />
		</data>
	</rules>

I cannot run this yet: https://github.com/TobiasNx/notWorkingFlux/tree/main/multipleColumnLookup

Results in :

{ }
{ }
{ }

Expected:

{"animal":"Mammal"}
{"animal":"Bird"}
{"animal":"Insect"}

@TobiasNx TobiasNx assigned blackwinter and TobiasNx and unassigned TobiasNx Oct 14, 2022
@TobiasNx TobiasNx assigned blackwinter and unassigned blackwinter and TobiasNx Oct 14, 2022
@blackwinter
Copy link
Member Author

blackwinter commented Oct 14, 2022

You have to specify the separator \t as character entity &#09; (or leave it off as it's the default anyway).

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Oct 14, 2022
@blackwinter
Copy link
Member Author

I am still not sure about where to add the options:

Options go on the filemap specification; Metamorph doesn't have the lookup "shortcut".

@TobiasNx
Copy link
Contributor

Okay, i tested it. It seems to work. Great. +1

Since I played around with the same file in different directions with sometimes duplicated values what I would recommend is to document that always the last match is selected: https://github.com/TobiasNx/notWorkingFlux/blob/60a00e538569527d88b69cc518d61b4b9ab9410e/multipleColumnLookup

Copy link
Contributor

@TobiasNx TobiasNx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 But document the selection of last match.

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Oct 17, 2022
@blackwinter
Copy link
Member Author

But document the selection of last match.

Do you want to provide a suggestion? This behaviour has not changed, it's always been like this.

TobiasNx added a commit that referenced this pull request Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants