Add support for new company attributes #183
Conversation
thewheat
force-pushed
the
timlim/company-setters
branch
from
17c71c4 to
e76c356
Compare
| * @param sessionCount | ||
| * @return the company object | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Needed to remove this as it was preventing tests from passing.
Open to alternative approaches to this
It is possible to remove setSessionCount method completely and Jackson will automatically read and extract the correct sessionCount value
thewheat
force-pushed
the
timlim/company-setters
branch
from
e76c356 to
794a361
Compare
| @Deprecated | ||
| public Company setSessionCount(int sessionCount) { | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Removed this as it
- prevented tests from passing
- and doesn't seem to do anything of use and is probably overriding default behaviour causing and is causing issues such as Company.getSessionCount() returning '0' in Java SDK #202 as well
| //noinspection RedundantIfStatement | ||
| if (userCount != null ? !userCount.equals(company.userCount) : company.userCount != null) return false; | ||
| if (size != company.size) return false; | ||
| if (website != null ? !website.equals(company.website) : company.website != null) return false; |
There was a problem hiding this comment.
This is quite complicated to read, you want to make sure that the current website and company website aren't equal, right? Is there any way you could export the equals checks to a separate method that returns the boolean?
There was a problem hiding this comment.
Yeah I can see that it is essentially checking for the possibly inequality between the 2 objects. I was following the standard format that exists for string checking and it is utilised in several places. Could definitely use a refactor though 👍
Do you see any issues with if (!Objects.equals(website, company.website)) return false; https://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#equals(java.lang.Object,%20java.lang.Object)
I can try implement that as it seems built but seems to require Java 1.7
There was a problem hiding this comment.
That's definitely a lot easier to read. What version of Java is this on? Why isn't it up higher?
That would definitely be nice, but I can understand that you're following the format. As well though, maintainability and readability are definitely important 😄
This definitely isn't a breaking change though.
There was a problem hiding this comment.
Don't think we have specified a version of Java we support but created this issue #214 to track the possibility of simplifying this
There was a problem hiding this comment.
Sorry Tim, I missed this. I think you're definitely right we need to look into simplifying it.
| @@ -0,0 +1,222 @@ | |||
| package io.intercom.api; | |||
There was a problem hiding this comment.
Delighted that you're adding these tests, but I'm not actually sure that they're running in CI? Taking a look I can't see any specific suites listed 🤔
SeanHealy33
left a comment
SeanHealy33
left a comment
There was a problem hiding this comment.
Chatted on Slack about this.
I think this pr is trying to do too much currently and would like 2-3 separate pr's for each issue.
thewheat
force-pushed
the
timlim/company-setters
branch
from
794a361 to
e20348b
Compare
thewheat
changed the title
|
@SeanHealy33 split out into
|
thewheat
force-pushed
the
timlim/company-setters
branch
from
054b2ac to
d36de74
Compare
jonnyom
left a comment
jonnyom
left a comment
There was a problem hiding this comment.
I'm happy with this, just curious about the lastRequestAt object and then I'll approve!
jonnyom
dismissed
SeanHealy33’s stale review
Tim has refined the PR to only work on company data
|
👍 nice work @thewheat |
Addresses #179
& #202- Add tests including