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

marshmallow: evaluate fields.Str vs SanitizedUnicode #53

Closed
ppanero opened this issue Feb 11, 2020 · 3 comments
Closed

marshmallow: evaluate fields.Str vs SanitizedUnicode #53

ppanero opened this issue Feb 11, 2020 · 3 comments
Labels

Comments

@ppanero
Copy link
Member

ppanero commented Feb 11, 2020

Several fields, such as type, are strings that can only take as value one of an enumeration. Therefore at the moment there is no need for them to be sanitized and could go along with just being a Str(). However, if we open these to customization (e.g. introduce your own CVs) it might cause problems (maybe in other languages that are not English).

Run some tests on performance from marshmallow.fields.Str vs invenio_records_rest.schemas.fields.SanitizedUnicode.

1 load/dump:

Sanitized: 0.0006556510925292969 seconds
Marshmallow Str: 0.0002028942108154297 seconds

1000 load/dumps:

Sanitized:  0.18001985549926758 seconds
Marshmallow Str: 0.09990668296813965 seconds

100000 load/dumps:

Sanitized: 18.859431743621826 seconds
Marshmallow Str: 10.20322585105896 seconds
@ppanero ppanero mentioned this issue Feb 11, 2020
@fenekku
Copy link
Contributor

fenekku commented Feb 27, 2020

Grooming
This should be tackled when we deal with #5

@github-actions
Copy link
Contributor

This issue was automatically marked as stale.

@github-actions github-actions bot added the stale label Jan 28, 2021
@fenekku
Copy link
Contributor

fenekku commented Jan 28, 2021

I think this can be closed. The bottom line:

  • if we know the string will never be used to hold potential internationalized string, then use Str (e.g. enums)
  • otherwise use SanitizedUnicode (e.g. free text, translated text)

@fenekku fenekku closed this as completed Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants