mighty-watcher
mighty-watcher copied to clipboard
IgorPerikov
Treat label everywhere as class, not string
Places to fix:
- https://github.com/IgorPerikov/mighty-watcher/blob/master/src/main/kotlin/com/github/igorperikov/mightywatcher/entity/SearchTask.kt
- https://github.com/IgorPerikov/mighty-watcher/blob/master/src/main/kotlin/com/github/igorperikov/mightywatcher/service/EasyLabelsStorage.kt
Do you mean interpretate as enum or something? Label looks more like external entity as for me, isn't it's better place for that in resources?
What I am saying is that sometimes in class signatures labels are passed as strings and somewhere as class Label, so maybe it makes sense to unify it, because it's the same logical entity
This seems pretty straight forward, but to clarify, in EasyLabelsStorage.kt we would now return a HashSet filled with the Label data object, right? If so, I'll make this PR later tonight.
@ggenya132 basically, yes. What I would like to point out is that there is some logic according to label equality checks:
Right now I explicitly call toLowerCase() on what is coming from github api call here:
https://github.com/IgorPerikov/mighty-watcher/blob/master/src/main/kotlin/com/github/igorperikov/mightywatcher/service/LabelsService.kt#L12
also it is expected (and covered with unit test) that EasyLabelsStorage.kt returns labels in lower case only
It would definitely makes sense to put this logic into a single place - maybe toLowerCase() everything that passed into Label.kt constructor or override it's equals/hashcode logic, what do you think?
@IgorPerikov I think that's a logical separation of concerns. Thanks for the feed back. Would the code look something like,
.map { new Label( it.name ) }
@ggenya132 on the EasyLabelsStorage side - yes, that's what I was thinking about
@IgorPerikov Ok great, appreciate it. Should be within my ability to do this, thanks for creating such a nice newbie issue for folks to tackle.