-
Notifications
You must be signed in to change notification settings - Fork 508
[network-creation]: Add createdAt field #791
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
base: main
Are you sure you want to change the base?
Conversation
|
Not sure if I added the Maybe something to do with the encoding and decoding done in the |
|
Not sure the protocol with pinging maintainers, as I don't want to bloat anyone's notifications. But @jglogan , I would love to get some 👀 on this. Mainly to check if I am even taking the right steps in what we want accomplish here. |
|
@saehejkang I'll have a look next week. |
|
@saehejkang I'll have a review of this done by the end of the day. The main thing I want to consider is whether what you have here follows a pattern that we can apply to the metadata for all our managed resources. |
| case .running(let configuration, _): configuration.id | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern could get out of hand as we add more configuration attributes.
Perhaps we could have a public var configuration: NetworkConfiguration? that provides configuration if the state has that associated data (always would for now but it would give us leeway for states that don't)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand what you kinda mean? Do we want to update id as well?
public var configuration: NetworkConfiguration? {
switch self {
case .created(let configuration): configuration
case .running(let configuration, _): configuration
}
}
public var createdAt: Date? {
configuration?.createdAt
}|
|
||
| public struct PrintableNetwork: Codable { | ||
| let id: String | ||
| let createdAt: Date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this in PrintableNetwork if we're not printing it for now, right?
Let's leave it out here and not print the create date in tabular output, to start with.
9fd5f2d to
638912c
Compare
| let container = try decoder.container(keyedBy: CodingKeys.self) | ||
|
|
||
| id = try container.decode(String.self, forKey: .id) | ||
| createdAt = try container.decode(Date.self, forKey: .createdAt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type of Change
Motivation and Context
Closes #665
Testing