Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

Conversation

frascuchon
Copy link
Member

Description

This PR adds a server validation to avoid creating several span questions using the same field.

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

  • New feature (non-breaking change which adds functionality)
  • Refactor (change restructuring the codebase without changing functionality)
  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

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

  • Test A
  • Test B

Checklist

  • I added relevant documentation
  • I followed 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/)

@frascuchon frascuchon requested a review from jfcalvo March 8, 2024 10:57
@frascuchon frascuchon force-pushed the feat/add-uniqueness-field-validation-for-span-questions branch from 7dbc917 to cfb72e5 Compare March 8, 2024 10:59
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.33%. Comparing base (5cd1019) to head (dfe0c34).

Additional details and impacted files
@@                       Coverage Diff                        @@
##           feat/support-for-span-questions      #60   +/-   ##
================================================================
  Coverage                            90.33%   90.33%           
================================================================
  Files                                  189      189           
  Lines                                 9289     9292    +3     
================================================================
+ Hits                                  8391     8394    +3     
  Misses                                 898      898           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jfcalvo jfcalvo left a comment

Choose a reason for hiding this comment

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

Looks good to me. I added some comments in the case that you want to improve it a little bit.

@@ -120,6 +120,10 @@ def _validate_span_question_settings_before_create(
if field not in field_names:
raise ValueError(f"'{field}' is not a valid field name.\nValid field names are {field_names!r}")

for question in dataset.questions:
if question.type == QuestionType.span and field == question.parsed_settings.field:
Copy link
Member

Choose a reason for hiding this comment

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

I already added in one commit a property to Question database model to check if if span or not. But after some changes I removed it because I was not using it.

Take a look to the commit here: 799743b#diff-b7efe64daa88a5d7c98088e2a66d38739605226e60d4ed967712ca2790c54931

If you want to apply that change then you can do:

Suggested change
if question.type == QuestionType.span and field == question.parsed_settings.field:
if question.is_span and field == question.parsed_settings.field:

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to keep it as is. We already have other places using this kind of condition with settings directly and it can be confusing seeing is_span or settings.type == QuestionType.span depending on the context.

We can review it later when using question validators. However, migrating the project to Python 3.10 and using the pattern-matching switch would be a better option.

tests/unit/api/v1/datasets/test_create_dataset_question.py Outdated Show resolved Hide resolved
Co-authored-by: José Francisco Calvo <jose@argilla.io>
@frascuchon frascuchon merged commit 1d9c1f1 into feat/support-for-span-questions Mar 8, 2024
1 of 2 checks passed
@frascuchon frascuchon deleted the feat/add-uniqueness-field-validation-for-span-questions branch March 8, 2024 13:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants