Skip to content

Default to SVG name for display for non-base DC#1956

Draft
n-h-diaz wants to merge 2 commits into
datacommonsorg:masterfrom
n-h-diaz:specialized_entity
Draft

Default to SVG name for display for non-base DC#1956
n-h-diaz wants to merge 2 commits into
datacommonsorg:masterfrom
n-h-diaz:specialized_entity

Conversation

@n-h-diaz

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new helper function isCustomHierarchy to identify SVGs from non-base Data Commons instances and integrates it into getSpecializedEntity alongside an empty check on childName. The reviewer suggests renaming isCustomHierarchy to isNonBaseDCHierarchy to prevent confusion with existing custom SVGs that start with "dc/", and adding an empty string check to the helper for robustness.

Comment on lines +816 to +819
// isCustomHierarchy checks if the SVG is part of a non-base DC.
func isCustomHierarchy(svg string) bool {
return !strings.HasPrefix(svg, "dc/")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The name isCustomHierarchy is potentially misleading because "Custom" is already used in the codebase (e.g., CustomSVGPrefix = "dc/g/Custom_") to refer to local custom SVGs, which do start with dc/. Since this function specifically checks for SVGs from a non-base Data Commons instance (which do not start with dc/), renaming it to isNonBaseDCHierarchy or isExternalHierarchy would be much clearer and prevent future confusion. Additionally, adding a check for an empty string makes the helper more robust.

Suggested change
// isCustomHierarchy checks if the SVG is part of a non-base DC.
func isCustomHierarchy(svg string) bool {
return !strings.HasPrefix(svg, "dc/")
}
// isNonBaseDCHierarchy checks if the SVG is part of a non-base DC.
func isNonBaseDCHierarchy(svg string) bool {
return svg != "" && !strings.HasPrefix(svg, "dc/")
}
References
  1. Ensure that function docstrings accurately reflect the function's signature and return values.

Comment on lines 822 to 825
func getSpecializedEntity(parent, child, childName string) string {
if !strings.Contains(child, "_") || isCuratedHierarchy(child) { // Child is likely curated.
if childName != "" && (!strings.Contains(child, "_") || isCuratedHierarchy(child) || isCustomHierarchy(child)) { // Child is likely curated.
return childName
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Update the call to use the renamed isNonBaseDCHierarchy function for clarity.

Suggested change
func getSpecializedEntity(parent, child, childName string) string {
if !strings.Contains(child, "_") || isCuratedHierarchy(child) { // Child is likely curated.
if childName != "" && (!strings.Contains(child, "_") || isCuratedHierarchy(child) || isCustomHierarchy(child)) { // Child is likely curated.
return childName
}
func getSpecializedEntity(parent, child, childName string) string {
if childName != "" && (!strings.Contains(child, "_") || isCuratedHierarchy(child) || isNonBaseDCHierarchy(child)) { // Child is likely curated.
return childName
}

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.

1 participant