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

fix: add missing question update validations #3967

Conversation

jfcalvo
Copy link
Member

@jfcalvo jfcalvo commented Oct 17, 2023

Description

This PR adds the following changes:

  • Question title and description are using the same validations for update and create.
  • Question settings is now a non nullable field when updating.
  • Metadata Property title and visible_for_annotators are now non nullable fields when updating.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

  • Running unit tests locally.
  • Running unit tests locally with PostgreSQL.

Checklist

  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

Comment on lines +317 to +323
QuestionName = Annotated[
constr(
regex=QUESTION_CREATE_NAME_REGEX,
min_length=QUESTION_CREATE_NAME_MIN_LENGTH,
max_length=QUESTION_CREATE_NAME_MAX_LENGTH,
)
title: constr(
),
PydanticField(..., description="The name of the question"),
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can move the regex, min_length and max_length to the PydanticField and use str instead of constr

Copy link
Member

Choose a reason for hiding this comment

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

at some point we're going to have to do it anyway: https://docs.pydantic.dev/latest/migration/#constrained-types

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should do it for all of the annotated fields once we move to v2.

@github-actions
Copy link

The URL of the deployed environment for this PR is https://argilla-quickstart-pr-3967-ki24f765kq-no.a.run.app

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
src/argilla/server/schemas/v1/datasets.py 99.07% <100.00%> (+<0.01%) ⬆️
...c/argilla/server/schemas/v1/metadata_properties.py 93.87% <100.00%> (+0.26%) ⬆️
src/argilla/server/schemas/v1/questions.py 96.87% <100.00%> (+0.04%) ⬆️

📢 Thoughts on this report? Let us know!.

@frascuchon frascuchon self-requested a review October 17, 2023 16:04
@jfcalvo jfcalvo merged commit 6ef1bf6 into feature/support-for-metadata-filtering-and-sorting Oct 17, 2023
@jfcalvo jfcalvo deleted the fix/fix-question-update-validations branch October 17, 2023 16:05
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.

3 participants