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

Add more descriptive names to unmapped datatypes in data dictionary #23

Closed
wants to merge 4 commits into from
Closed

Add more descriptive names to unmapped datatypes in data dictionary #23

wants to merge 4 commits into from

Conversation

foster33
Copy link
Collaborator

@foster33 foster33 commented Apr 23, 2024

This PR closes DW-Issue-708

Depends on dw-type-utils PR 33

The data dictionary page previously showed "Unknown" for data types that were not in its list of datatypes.

This PR adds:

  • Use of a class defined data dictionary description for 'Types'
  • Adds some new test cases

@foster33 foster33 changed the title WIP: Add more descriptive names to unmapped datatypes in data dictionary Add more descriptive names to unmapped datatypes in data dictionary Apr 29, 2024
@foster33
Copy link
Collaborator Author

An example of this change can be seen below:

Before the change:
dw-708-PR-screenshot-before
Both types shown above are 'Unknown'

After the change:
dw-708-PR-screenshot
The types now show as 'Date'.

Copy link
Collaborator

@jwomeara jwomeara left a comment

Choose a reason for hiding this comment

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

Interesting... why would we go with this approach vs. adding intentional descriptions for all of the available types in the data dictionary configuration? I'm referring to the map of types to descriptions found in DataDictionaryProperties.

@foster33
Copy link
Collaborator Author

Interesting... why would we go with this approach vs. adding intentional descriptions for all of the available types in the data dictionary configuration? I'm referring to the map of types to descriptions found in DataDictionaryProperties.

@jwomeara I personally do not think that this change is intended to be used in replacement of explicitly adding type descriptions in the dictionary configuration. I see it more as removing the 'Unknown' verbiage and replacing it with clearer information in the event that the type is not added to the map for whatever reason (what it was doing originally). So, I agree that the missing types should be added to the config, but I see this as different.

@ivakegg
Copy link
Collaborator

ivakegg commented Apr 30, 2024

Interesting... why would we go with this approach vs. adding intentional descriptions for all of the available types in the data dictionary configuration? I'm referring to the map of types to descriptions found in DataDictionaryProperties.

Yea, I am unsure why we went that way as well. I think Brian may have been the originator. I guess this was an attempt to make it prettier somehow. I would rather have a method on the type itself that returns a pretty name and not have this hardcoded map here. This PR does answer the call of the ticket. The question is whether it is worth going farther or removing this map altogether.

@jwomeara
Copy link
Collaborator

Building the description into the type class itself makes the most sense to me, and should be a relatively simple update. I guess I'm ok with this as a fallback - I just don't want to confuse users as this is going to expose them to some datawave internals that they might not have seen before. For example, picking from some of our available types, I think seeing things like the following could potentially be confusing for users: TrimLeadingZeros, RawDate, NoOp, Lc, LcNoDiacritics, HitTerm to name a few...

@foster33
Copy link
Collaborator Author

foster33 commented May 6, 2024

See dw-type-utils PR 33 's comments for more information on the type descriptions being used.

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

Successfully merging this pull request may close these issues.

unmapped datatypes show as Unknown in data dictionary
3 participants