-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixes Data type mismatch issue on LeaderBoard #349
Conversation
@Brutus5000 Can you help me out with this ? |
@iamTheL If you rebase on develop it will be fixed. |
public GlobalLeaderboardEntry getGlobalEntry(int playerId) { | ||
return globalLeaderboardRepository.findByPlayerId(playerId); | ||
public GlobalLeaderboardEntry getGlobalEntry(String playerId) { | ||
if(DataTypeValidation.isNumeric(playerId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe with Brackets. I actually just read your guidelines and I can not find anything on that but we like to keep code style alike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added the brackets for the if-else
block. Still it seems to be failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to find out what is causing the test coverage to drop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if u add more lines without writing tests the coverage increases so if u write new stuff(like u did) test it. Make a unit test feed the Service a string and see if it throws the Exception u expected...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will write tests and submit again.
src/main/java/com/faforever/api/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/faforever/api/leaderboard/LeaderboardService.java
Outdated
Show resolved
Hide resolved
your suggested fix does not work.(tested it) This should also be way more compact.
And use StringUtils from Apache that is it basically. |
@axel1200 I went ahead with your suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry to annoy you further but this would be a something that can be easily tested, either unit or integration test. Does not really matter.
Got superseeded by #372 as we replaced the whole exception. |
#302