Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat label everywhere as class, not string #52

Open
IgorPerikov opened this issue Sep 13, 2019 · 8 comments
Open

Treat label everywhere as class, not string #52

IgorPerikov opened this issue Sep 13, 2019 · 8 comments

Comments

@bvc3at
Copy link
Contributor

bvc3at commented Sep 24, 2019

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
Copy link
Owner Author

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
Copy link

ggenya132 commented Sep 25, 2019

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
Copy link
Owner Author

@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
Copy link

ggenya132 commented Sep 26, 2019

@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
Copy link
Owner Author

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

@ggenya132
Copy link

@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
Copy link
Owner Author

IgorPerikov commented Oct 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants