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 helper for getting resource descriptors #6

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

johnatawnclementawn
Copy link
Member

@johnatawnclementawn johnatawnclementawn commented Nov 26, 2024

Placeholder pr - could become a function that returns strings from localized objects based on system language code or preferred language code.

Base automatically changed from jtw/dont-raise-error-for-no-labels to main November 27, 2024 17:01
@johnatawnclementawn johnatawnclementawn marked this pull request as ready for review December 2, 2024 14:35
Copy link

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

👋 ! Just some clarification questions on my part

arches_vue_utils/src/arches_vue_utils/utils.ts Outdated Show resolved Hide resolved
systemLanguageCode: string,
): [string, string] => {
if (!data) {
return ["None", "None"];
Copy link

Choose a reason for hiding this comment

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

Why not throw an error here? ["None", "None"] seems like an odd thing to return

Copy link
Member Author

@johnatawnclementawn johnatawnclementawn Dec 2, 2024

Choose a reason for hiding this comment

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

I was assuming (perhaps wrongly) that my suggestion in #5 would be implemented and was following that pattern. However, as it stands now, In getItemLabel above, we return empty string if the label isn't found. I would imagine that it's valid for resources to not always have descriptors (depending on the data model), so we should provide a fall-back value (whether "" or "None") rather than erroring?

Copy link

@chrabyrd chrabyrd Dec 3, 2024

Choose a reason for hiding this comment

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

I guess a "None" string is a bit odd for me. I'd either expect this to be null, or an empty string.

As far as if it's better to throw an error or not -- I'm not sure. I'm happy to move forward with fallback values and change later if it ends up biting us

@johnatawnclementawn johnatawnclementawn marked this pull request as draft December 4, 2024 19:22
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.

2 participants