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

Show uniqueness in step KW field #3786. #4379

Merged
merged 3 commits into from Aug 22, 2017

Conversation

Projects
None yet
2 participants
@ismailsunni
Member

ismailsunni commented Aug 21, 2017

What does it fix?

  • Ticket: #3786
  • Funded by: DFAT
  • Description:
    Show uniqueness in step KW field #3786.

screen shot 2017-08-21 at 17 54 01

Adding in the InaSAFE fields wizard is more complex.

@ismailsunni ismailsunni requested a review from Gustry Aug 21, 2017

@ismailsunni

This comment has been minimized.

Member

ismailsunni commented Aug 21, 2017

@Gustry if it's LGTM, go on to merge it. And update transifex for the new strings. It's just 3 simple strings.

@Gustry

Looks nearly good.
Your screenshot is a hazard layer, it doesn't make any sense here, but more an aggregation layer as we need to select a unique column. So I guess, it's the same step but anyway. Thanks. There is something to fix.

@@ -124,6 +125,9 @@ def on_lstFields_itemSelectionChanged(self):
if not isinstance(field_names, list):
field_names = [field_names]
field_descriptions = ''
LOGGER.debug('Test')
feature_count = self.parent.layer.featureCount()
LOGGER.debug('Feature count: %s' % feature_count)

This comment has been minimized.

@Gustry

Gustry Aug 21, 2017

Contributor

Can we remove these debug noise?

unique_values_str = ', '.join(unique_values_str)
field_descriptions += tr('<b>Field name</b>: {field_name}').format(
field_name=field_name)
field_descriptions += tr(
'<br><b>Field type</b>: {field_type}').format(
field_type=field_type)
if len(unique_values) == feature_count:

This comment has been minimized.

@Gustry

Gustry Aug 21, 2017

Contributor

Some providers won't be able to provide the feature count. For instance, the feature count on an OSM file will always return -1, because we can't count features in an OSM file efficiently. So can we skip this block if it's -1? It would not make sense to display from -1 features.

screen shot 2017-08-21 at 13 11 59

This comment has been minimized.

@ismailsunni

ismailsunni Aug 22, 2017

Member

Ouch, I never know about it. I will skip if the count == -1.

Thanks

@ismailsunni

This comment has been minimized.

Member

ismailsunni commented Aug 22, 2017

Your screenshot is a hazard layer, it doesn't make any sense here, but more an aggregation layer as we need to select a unique column. So I guess, it's the same step but anyway. Thanks. There is something to fix.

Yes, it just example. I will make it to show only on aggregation layer.

@ismailsunni

This comment has been minimized.

Member

ismailsunni commented Aug 22, 2017

@Gustry done
screen shot 2017-08-22 at 08 22 56

@Gustry

Gustry approved these changes Aug 22, 2017

LGTM

else:
unique = tr('No')
field_descriptions += tr(
'<br><b>Unique</b>: %s (%d unique values from %d '

This comment has been minimized.

@Gustry

Gustry Aug 22, 2017

Contributor

Can we switch to the new format: https://pyformat.info/#simple ?
I thought we made it deprecated to use the old way for string formating.

This comment has been minimized.

@ismailsunni

ismailsunni Aug 22, 2017

Member

Shame, of course. I always support to use new format, but it seems I forget about it.
Thanks

@Gustry

This comment has been minimized.

Contributor

Gustry commented Aug 22, 2017

Thanks, it's good!

Can you squash them?

@ismailsunni

This comment has been minimized.

Member

ismailsunni commented Aug 22, 2017

@Gustry Yes, I am planning to squash them... Thanks

@ismailsunni ismailsunni merged commit d9c2a3c into inasafe:develop Aug 22, 2017

3 checks passed

ci/sideci Meow! No issues found. It's clean code!
Details
code-quality/landscape Code quality remained the same
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ismailsunni ismailsunni deleted the ismailsunni:fix_3786 branch Sep 4, 2017

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