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

Common tags #549

Merged
merged 5 commits into from
Dec 4, 2019
Merged

Common tags #549

merged 5 commits into from
Dec 4, 2019

Conversation

mikkokar
Copy link
Contributor

@mikkokar mikkokar commented Dec 2, 2019

Description

Introduces a CommonTag class to simplify and harmonise Styx object tags API. Now each Tag will be represented as a CommonTag instance, with provided name, encode and decode functions.

This imposes a common format: name = tag-value.

Each tag is associated with value type T. The encode and decode functions convert between values of T and encoded tag value strings.

All common tag operations, like creating, searching, matching, and removing tags are now implemented as higher order functions in the common base class.

Alternative API

Some of the common functions could be implemented as Kotlin extension methods. Which method do you prefer? Let me know and we can shape the code accordingly.

An alternative to CommonTags find method would be:

fun <T> Set<String>.valueOf(tag: CommonValueTag<T>): T? = this.firstOrNull { tag.match(it) }
        ?.let { tag.valuePart(it) }
        ?.let { tag.decode(it) }

Similarly, an alternative to match would be:

fun String.isA(tag: CommonValueTag<*>) = tag.match(this)

An alternative to valueOf method would be:

fun <T> String.match(tag: CommonValueTag<T>) = tag.valuePart(this)
         ?.let { tag.decode(it) }

@mikkokar mikkokar marked this pull request as ready for review December 2, 2019 16:56
Rename few files.
Copy link
Contributor

@chrisgresty chrisgresty left a comment

Choose a reason for hiding this comment

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

A few small observations, nothing particularly significant.

Comment on lines +37 to +38
val encode: (T) -> String?,
val decode: (String) -> T?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth providing defaults for encode and decode as { it } ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this possible without knowing the type T?

@mikkokar mikkokar merged commit 7235bb3 into ExpediaGroup:master Dec 4, 2019
@mikkokar mikkokar deleted the common-tags branch December 4, 2019 07:41
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 this pull request may close these issues.

2 participants