Skip to content
This repository was archived by the owner on Feb 25, 2023. It is now read-only.

Conversation

@0xB10C
Copy link
Contributor

@0xB10C 0xB10C commented Dec 4, 2019

This PR adds the foundation and groundwork for the state and description codes. The handling by the Supervisor and the Middleware will come in part 2.

I leave this as a draft until BitBoxSwiss/bitbox02-api-go#17 is merged

This adds the functionallity to add, remove or get top elements from a
Redis sorted set.
@0xB10C 0xB10C added the P:1 label Dec 4, 2019
@0xB10C 0xB10C requested review from Stadicus and Tomasvrba December 4, 2019 12:52
@0xB10C 0xB10C self-assigned this Dec 4, 2019
@0xB10C 0xB10C force-pushed the 2019-12-state-codes-part1 branch from 73e6c6c to c900e6a Compare December 4, 2019 14:10
@0xB10C 0xB10C marked this pull request as ready for review December 4, 2019 14:17
Copy link
Collaborator

@Stadicus Stadicus left a comment

Choose a reason for hiding this comment

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

Concept ACK, untested and no code review

@0xB10C 0xB10C force-pushed the 2019-12-state-codes-part1 branch from c900e6a to 149780d Compare December 4, 2019 14:23
@0xB10C
Copy link
Contributor Author

0xB10C commented Dec 4, 2019

fix typo correspointing -> corresponding

Copy link
Contributor

@Tomasvrba Tomasvrba left a comment

Choose a reason for hiding this comment

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

Light code review, I need caffeine XD

This changes the supervisor to use the Redis HTTP query interface
inmplementation from the middleware.
This adds the definitions for:
- a map defining the priority of a descriptionCode
- a map defining the stateCode of a descriptionCode
- a couple of LogTags the middleware logs, which are read by the
supervisor
This adds a Redis string for the Base stateCode and a Redis sorted set
for the descriptionCode.
@0xB10C 0xB10C force-pushed the 2019-12-state-codes-part1 branch from 149780d to 71c173f Compare December 4, 2019 16:01
@0xB10C
Copy link
Contributor Author

0xB10C commented Dec 4, 2019

Thanks @Tomasvrba! Addressed.

@Tomasvrba Tomasvrba self-requested a review December 5, 2019 13:07
Copy link
Contributor

@Tomasvrba Tomasvrba left a comment

Choose a reason for hiding this comment

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

ACK with two small comments:

  1. When reviewing sequentially commit by commit, it's helpful when the commit message says why the commit does something, not what it does. For the first commit:

add: Redis sorted-set add, remove, gettop element
This adds the functionallity to add, remove or get top elements from a
Redis sorted set.

I can see from the code that it adds the functionallity to add, remove or get top elements from a Redis sorted set, but without looking at the rest of the commits in the PR first, it would be good to know why this is needed.

  1. Another why, what's the logic behind the MapDescriptionCodePriority numbers?

@0xB10C
Copy link
Contributor Author

0xB10C commented Dec 5, 2019

Thank you Tom!

For context:
I originally planned to have one big PR with a detailed documentation and more context, but had to split it up into two due to interlinked Go Module dependencies for two projects in the same repo. Forgot to add the reasoning here. Will hopefully make more sense in the second PR.

I thought about answering your questions here, but I guess it makes more sense to link to the complete documentation in the second PR #305

@0xB10C 0xB10C merged commit 4c6c9f9 into BitBoxSwiss:master Dec 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants