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

Check for valid name property value in MapWayname #1031

Merged
merged 1 commit into from
Jun 15, 2018
Merged

Conversation

danesfeder
Copy link
Contributor

Closes #1029

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing not blocking the PR but definitely worth 👀

@@ -72,6 +73,20 @@ public void onRoadsReturnedFromQuery_visibilityIsSetToTrueAndLayoutIconAdded() {
verify(waynameLayer, times(1)).setProperties(any(PropertyValue.class));
}

@Test
public void onFeatureWithoutNamePropertyReturned_updateIsIgnored() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@@ -71,12 +71,15 @@ public void updateDefaultMapTopPadding(int topPadding) {

private void updateLayerWithRoadLabelFeatures(List<Feature> roads, SymbolLayer waynameLayer) {
if (!roads.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO updateLayerWithRoadLabelFeatures is getting too long.
What about extracting blocks of code into well-named private methods and simplifying conditional expressions somehow?

@danesfeder danesfeder merged commit b48a348 into master Jun 15, 2018
@danesfeder danesfeder deleted the dan-wayname-npe branch June 15, 2018 17:12
@danesfeder danesfeder mentioned this pull request Jun 21, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants