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

remove Logger.metadata property #36

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Conversation

weissi
Copy link
Member

@weissi weissi commented Apr 8, 2019

Motivation:

Logger had two ways to access the metadata:

logger[metadataKey: "foo"] = "bar"

and

logger.metadata["foo"] = "bar"

. The first one is the preferred API (as it doesn't force the
LogHandlers to store it in a dictionary) but the second one was still
available for cases where you need to import/export the whole metadata
storage.

Instead of exposing this as a dictionary, we should make the
'whole-metadata import/export' opaque. We're close to tagging 1.0.0
however let's design this API properly and add a sensible implementation
for 1.1.0 or so.

Moditications:

remove Logger.metadata property

Result:

Motivation:

Logger had two ways to access the metadata:

    logger[metadataKey: "foo"] = "bar"

and

    logger.metadata["foo"] = "bar"

. The first one is the preferred API (as it doesn't force the
`LogHandler`s to store it in a dictionary) but the second one was still
available for cases where you need to import/export the whole metadata
storage.

Instead of exposing this as a dictionary, we should make the
'whole-metadata import/export' opaque. We're close to tagging 1.0.0
however let's design this API properly and add a sensible implementation
for 1.1.0 or so.

Moditications:

remove `Logger.metadata` property

Result:

- We can design the import/export APIs and add them when they're ready.
- the `LogHandler` API is unchanged
@weissi weissi requested a review from tomerd April 8, 2019 11:02
@ktoso
Copy link
Member

ktoso commented Apr 8, 2019

Instead of exposing this as a dictionary, we should make the 'whole-metadata import/export' opaque.

To sanity check my understanding of the phrase: We'll want to design a type that is "the metadata container" that could be then implemented in various ways, but not forcing it to be a dict?

We're close to tagging 1.0.0 however let's design this API properly and add a sensible implementation for 1.1.0 or so.

Agreed with cutting out and designing well later for 1.1.0, I think I'd definitely have use cases for it (can open the ticket with an example use case).

@ktoso
Copy link
Member

ktoso commented Apr 8, 2019

Btw, how are we treating APIs in sandbox mode with regards to source compat, e.g. if we suddenly find out that we have to change the way LogHandler has this defined? As with this PR it still has the var metadata: Logger.Metadata { get set }, would we end up calling a 2.0 if we had to remove it?

@weissi
Copy link
Member Author

weissi commented Apr 8, 2019

Btw, how are we treating APIs in sandbox mode with regards to source compat, e.g. if we suddenly find out that we have to change the way LogHandler has this defined? As with this PR it still has the var metadata: Logger.Metadata { get set }, would we end up calling a 2.0 if we had to remove it?

Yes, we follow SemVer, so changing an existing API would mean 2.0.0 but that's okay I think. We could go along way with deprecations and back-filling old APIs with default implementations I think.

@weissi
Copy link
Member Author

weissi commented Apr 8, 2019

Instead of exposing this as a dictionary, we should make the 'whole-metadata import/export' opaque.

To sanity check my understanding of the phrase: We'll want to design a type that is "the metadata container" that could be then implemented in various ways, but not forcing it to be a dict?

We're close to tagging 1.0.0 however let's design this API properly and add a sensible implementation for 1.1.0 or so.

Agreed with cutting out and designing well later for 1.1.0, I think I'd definitely have use cases for it (can open the ticket with an example use case).

Yes, totally agreed that there are use-cases, I hope to bring it back in 1.1.0 or so

@ktoso
Copy link
Member

ktoso commented Apr 8, 2019

Cool, thanks for confirming. Happy to bump major even during sandbox time, no reason not to -- otherwise people and projects end up with "2.0 fear" 😜 👍

@weissi
Copy link
Member Author

weissi commented Apr 8, 2019

Happy to bump major even during sandbox time, no reason not to -- otherwise people and projects end up with "2.0 fear" 😜 👍

Agreed. We talked about it in the SSWG meeting and the idea is that the maturity levels and the version numbers are totally orthogonal. The only meaning attached to 1.0.0 is "we follow SemVer"

Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

looks good, let’s add an issue to track enabling mdc use cases

@ktoso
Copy link
Member

ktoso commented Apr 8, 2019

looks good, let’s add an issue to track enabling mdc use cases

Already here: #37 👍

@weissi weissi merged commit ca922e3 into apple:master Apr 8, 2019
@weissi weissi deleted the jw-remove-metadata branch April 8, 2019 19:28
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.

take out metadata to start with and make opaque later
3 participants