Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Sep 27, 2024

Make the specification of what Lifetime message is used for as clear and precise as possible.

Fixes #250.

@llucax llucax requested a review from a team as a code owner September 27, 2024 12:01
@github-actions github-actions bot added the part:protobuf Affects the protocol buffer definition files label Sep 27, 2024
@llucax llucax added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Sep 27, 2024
@llucax
Copy link
Contributor Author

llucax commented Sep 27, 2024

Skipping release notes, I think they are not necessary for such a small documentation change.

Comment on lines 24 to 25
// If the `end_timestamp` is not set, the asset is considered to be in
// operation until the end of the system's operational history.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "end of history" part reads a bit odd, because history indicates past tense. I feel that phrase does not add anything here, and can be removed:

Suggested change
// If the `end_timestamp` is not set, the asset is considered to be in
// operation until the end of the system's operational history.
// If the `end_timestamp` is not set, the asset is considered to be in
// operation.

The same comment applies to other instances where end-timestamp is being documented.

Copy link
Contributor Author

@llucax llucax Sep 27, 2024

Choose a reason for hiding this comment

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

What about "... is considered to be in operation indefinitely in the future"? I would like to make it explicit that it is in operation now but also at any time in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Should then be "indefinitely into the future".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Make the specification of what `Lifetime` message is used for as clear
and precise as possible.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Copy link
Contributor Author

llucax commented Sep 30, 2024

Enabling auto-merge.

@llucax llucax enabled auto-merge September 30, 2024 10:37
@llucax llucax self-assigned this Sep 30, 2024
Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM!

@llucax llucax added this pull request to the merge queue Sep 30, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit 3d0aa69 Sep 30, 2024
11 checks passed
@llucax llucax deleted the lifetime-docs branch September 30, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:protobuf Affects the protocol buffer definition files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add more specifications for Lifetime fields

2 participants