Skip to content

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Sep 7, 2022

Replaced some "todo" comments with something more meaningful.

No changelog necessary, IMO.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@battermann battermann temporarily deployed to cachix September 7, 2022 14:20 Inactive
@battermann battermann temporarily deployed to cachix September 7, 2022 14:20 Inactive
modify' (insertConfig iw)
NewHandle _tid -> pure $ IdPHandle "IdP 1" --todo(leif): generate a new handle
NewHandle _tid ->
-- Same handle for all IdPs is good enough, for now
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we elaborate on this? Why is a hardcoded handle fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is just a label shown to the team admin in the UI. IMO uniqueness is irrelevant for tests,

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 we'll see right away if this is not enough any more in the future, and then we can build something more sophisticated.

modify' (insertConfig iw)
NewHandle _tid -> pure $ IdPHandle "IdP 1" --todo(leif): generate a new handle
NewHandle _tid ->
-- Same handle for all IdPs is good enough, for now
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 we'll see right away if this is not enough any more in the future, and then we can build something more sophisticated.

@fisx
Copy link
Contributor

fisx commented Sep 8, 2022

No changelog necessary, IMO.

it would not have been more work to write the changelog entry instead of this line. and it wouldn't have prompted me to make things worse! :-)

anyway, fine.

@battermann
Copy link
Contributor Author

No changelog necessary, IMO.

it would not have been more work to write the changelog entry instead of this line. and it wouldn't have prompted me to make things worse! :-)

anyway, fine.

IMO not a matter of which is more or less work, but rather what provides more value. Do we really need to document this in a changelog? What are the benefits? And how do they weigh against unnecessary noise in the changelog?

@battermann battermann merged commit 1826a76 into develop Sep 8, 2022
@battermann battermann deleted the batterman/remove-change-todo-comments branch September 8, 2022 08:39
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.

5 participants