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

Add uri_encode (#273) #314

Merged
merged 5 commits into from
Jun 16, 2023
Merged

Add uri_encode (#273) #314

merged 5 commits into from
Jun 16, 2023

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented Jun 16, 2023

Use metamorph's URLEncoder for uri_encode.

Supersedes #285.

Use metamorph's URLEncoder for uri_encode.
uri_encode {
@Override
public void apply(final Metafix metafix, final Record record, final List<String> params, final Map<String, String> options) {
URL_ENCODER.setPlusForSpace(false);
Copy link
Member

Choose a reason for hiding this comment

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

Preferrably, we would expose this as an option plus_for_space (along with safe_chars). But I don't think it's necessarily a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - we can, if needed, easily enhance to use options.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but I'll unresolve it as a reminder.

@blackwinter blackwinter assigned dr0i and unassigned blackwinter Jun 16, 2023
@dr0i dr0i requested a review from blackwinter June 16, 2023 14:24
@blackwinter
Copy link
Member

Would you also document this new function in the README?

@dr0i
Copy link
Member Author

dr0i commented Jun 16, 2023

Updated the README.

@blackwinter
Copy link
Member

Now we're entering shaky territory ;) We differentiate between "record-level" and "field-level" methods. You placed the implementation in the latter section, but the documentation in the former. I don't think we have clear criteria for which is which (@fsteeg, @TobiasNx?), but this one is almost definitely "field-level" I would say (so the documentation should match the implementation).

@dr0i
Copy link
Member Author

dr0i commented Jun 16, 2023

No, makes sense. Wasn't aware of two sections. Moved docu snippet to the proper place. Please have a look again.

@blackwinter
Copy link
Member

blackwinter commented Jun 16, 2023

Sorry, but now it's in "script-level" ;) I've moved it down in db84fb7b3420ec.

@dr0i
Copy link
Member Author

dr0i commented Jun 16, 2023

Ahhh - I put it into Script-level functions ... will fix that.

@blackwinter
Copy link
Member

blackwinter commented Jun 16, 2023

And I named the wrong section in the commit message :( Probably started the weekend a little early ;)

Fixed it in b3420ec and force-pushed. Hope that's okay.

@dr0i
Copy link
Member Author

dr0i commented Jun 16, 2023

Hope that's okay.

timely :)
Going to merge now.

@dr0i dr0i merged commit 2294a74 into master Jun 16, 2023
1 check passed
@dr0i dr0i deleted the 273-addUriEncodeFunction branch June 16, 2023 15:34
dr0i referenced this pull request in hbz/lobid-organisations Jul 4, 2023
This creates strange URIs. Probably needs to be fixed in metafacture-fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants