Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[#257] Provision to allow overriding concepts' datatype to 'Numeric'. #258

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

Ruhanga
Copy link
Member

@Ruhanga Ruhanga commented Dec 7, 2023

Issue: #257

@Ruhanga Ruhanga requested review from mseaton, mks-d and ibacher December 7, 2023 12:30
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Code here looks good, but I'm a little hesitant about the process described in the ticket. In particular, if a concept has the wrong datatype, the right way to handle that is to retire the concept with the wrong datatype and create a new concept with the right datatype, rather than simply changing the existing concept.

@Ruhanga
Copy link
Member Author

Ruhanga commented Dec 7, 2023

I agree especially if the concept was already in use by some Obs. There are those cases however where concepts can have their datatypes changed. We have this check here that enforces validity to change a concept's datatype.

Copy link
Collaborator

@mseaton mseaton left a comment

Choose a reason for hiding this comment

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

This looks right to me. Good catch

@mks-d mks-d changed the title [#257] Provided for overriding concepts' datatype to Numeric datatype. [#257] Provision to overriding concepts' datatype to 'Numeric'. Dec 7, 2023
@mks-d mks-d changed the title [#257] Provision to overriding concepts' datatype to 'Numeric'. [#257] Provision to allow overriding concepts' datatype to 'Numeric'. Dec 7, 2023
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

Approving based on others' approvals :-)

@mks-d mks-d merged commit 11d7c5c into mekomsolutions:master Dec 7, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants