IgorPerikov / mighty-watcher

Open source made easier
https://itnext.io/open-source-made-easier-aab8aebd849f
MIT License
47 stars 7 forks source link

Treat label everywhere as class, not string #52

Open IgorPerikov opened 5 years ago

IgorPerikov commented 5 years ago

Places to fix:

bvc3at commented 5 years ago

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?

IgorPerikov commented 5 years ago

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

ggenya132 commented 5 years ago

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.

IgorPerikov commented 5 years ago

@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?

ggenya132 commented 5 years ago

@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 ) } 
IgorPerikov commented 5 years ago

@ggenya132 on the EasyLabelsStorage side - yes, that's what I was thinking about

ggenya132 commented 5 years ago

@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.

IgorPerikov commented 5 years ago

@ggenya132 how's your progress so far? need help?