refactor(v3): add role slice (DEV-1010) #2099
Conversation
mpro7
changed the title
| * @param r the role to write | ||
| * @return Unit | ||
| */ | ||
| def storeRole(r: Role): UIO[RoleId] |
There was a problem hiding this comment.
| def storeRole(r: Role): UIO[RoleId] | |
| def storeRole(role: Role): UIO[RoleId] |
I think a little more expressive variable names are nicer
There was a problem hiding this comment.
In strongly typed languages the type of param, if not omitted, speaks for itself. Therefore in most of the cases instead of repeating the type in the name it could be shortened to first letter of the type, or named: value, param, whatever, because at the end the IDE will inform one need to pass something of the defined (Role in this case) type.
| val View: String = "view" | ||
| val Create: String = "create" | ||
| val Modify: String = "modify" | ||
| val Delete: String = "delete" | ||
| val Admin: String = "admin" | ||
| val availablePermissions: Set[String] = Set( | ||
| View, | ||
| Create, | ||
| Modify, | ||
| Delete, | ||
| Admin | ||
| ) |
There was a problem hiding this comment.
we should ad tome point agree how we want to model things like that, that are essentially like an enum
There was a problem hiding this comment.
What's wrong with snipped above?
| * @param r the role to write | ||
| * @return Unit | ||
| */ | ||
| def storeRole(r: Role): UIO[RoleId] |
| /** | ||
| * To be implemented... | ||
| */ | ||
| object placeholder {} |
There was a problem hiding this comment.
Is this file needed? Are you going to implement it? I don't think we need to test the repo api (it's just an interface). I think it makes more sense to test the implementations of it.
There was a problem hiding this comment.
Well this I'm not sure. You also have it in the user slice.
dsp-role/core/src/test/scala/dsp/role/sharedtestdata/RoleTestData.scala
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| val roleRepoMockTests = suite("RoleRepoMock")( | ||
| roleTests |
There was a problem hiding this comment.
I assume this roleTest is here on purpose (as placeholder)? If not, it would be wrong and should maybe be deleted (or real tests for the mock repo added).
There was a problem hiding this comment.
Sorry I don't get this comment.
| * @param value the [[String]] the value to be validated | ||
| * @param isoCode thelanguage ISO code to be validated | ||
| */ | ||
| sealed abstract case class LangString private (value: String, isoCode: String) |
There was a problem hiding this comment.
I think it would be more intuititve to turn the paramters' order around: have the isoCode and then the value. This would be the same order as the value is represented when given by or returned to the user, e.g. {"en": "some value"}.
|
Kudos, SonarCloud Quality Gate passed!
|
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: DEV-1010
What is the new behavior?
Does this PR introduce a breaking change?
Other information