Skip to content

Issue 52592: resolve correct unassigned samples folder column #6598

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

Merged
merged 1 commit into from
Apr 24, 2025

Conversation

labkey-nicka
Copy link
Contributor

@labkey-nicka labkey-nicka commented Apr 23, 2025

@labkey-nicka labkey-nicka requested a review from XingY April 23, 2025 19:48
@labkey-nicka labkey-nicka self-assigned this Apr 23, 2025
Copy link
Contributor

@XingY XingY left a comment

Choose a reason for hiding this comment

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

Not sure how I feel about this... a bit overkill?

@labkey-nicka
Copy link
Contributor Author

Not sure how I feel about this... a bit overkill?

How so?

@XingY
Copy link
Contributor

XingY commented Apr 24, 2025

Not sure how I feel about this... a bit overkill?

How so?

I understand the purpose is to enforcing the correct column is used. But still, this is a pretty straightforward util that's only used by that Inventory table. Could we instead have this util in InventoryQuerySchema?
private FieldKey getMaterialFieldKey(ExpMaterialTable.Column materialCol) { return FieldKey.fromParts(materialCol.name()); }

@labkey-nicka
Copy link
Contributor Author

I understand the purpose is to enforcing the correct column is used. But still, this is a pretty straightforward util that's only used by that Inventory table. Could we instead have this util in InventoryQuerySchema? private FieldKey getMaterialFieldKey(ExpMaterialTable.Column materialCol) { return FieldKey.fromParts(materialCol.name()); }

I see. What I'm doing here is just extending a notion that is already in place for other column enumerations. Here are a few examples:

This utility could already apply to many usages in platform but I didn't update those here since it's not particularly relevant.

@labkey-nicka labkey-nicka force-pushed the fb_unassigned_storage branch from e348fdd to fedf819 Compare April 24, 2025 21:17
@labkey-nicka labkey-nicka merged commit 4b94f69 into develop Apr 24, 2025
1 of 6 checks passed
@labkey-nicka labkey-nicka deleted the fb_unassigned_storage branch April 24, 2025 21:33
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