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

Enable CharWidget to return string #1623

Conversation

matthewhegarty
Copy link
Contributor

@matthewhegarty matthewhegarty commented Sep 13, 2023

Problem

Issue #1573.

This adds options to force CharWidget to return its values as strings. This seems logical because the other widgets use clean() to return their values as underlying types.

However, this could break existing implementations, so I have preserved the current default behaviour, and made it optional to return values as strings, also handling the 'null' case either as empty string or as None.

Maybe we will make this mandatory behaviour of CharWidget in a future release.

Solution

  • Updated constructor to accept kwargs
  • Modified clean()

Acceptance Criteria

Full unit tests added.

@matthewhegarty matthewhegarty linked an issue Sep 13, 2023 that may be closed by this pull request
Copy link

@pokken-magic pokken-magic left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense to me as an option. It might make sense to create it as a Resource level field that supplies this to all charwidgets as well - I always find it helpful to be able to set global values. It's especially useful for Excel and CSV that don't really have the same concept of null as Json and Yaml do.

Not 100% sure what the pattern should be for things with types like json and Yaml in the future, but it might make sense for the default behavior to only be for CSV/Excel formats to make strings.

@matthewhegarty
Copy link
Contributor Author

Hi Ryan
Thanks for reviewing so quickly. Some great points you make there.

It might make sense to create it as a Resource level field that supplies this to all charwidgets as well - I always find it helpful to be able to set global values.

Yes, I think I would implement this at the settings level (in fact, that might be better than as kwargs because like you say it would likely be a global setting). Although I think having both would give the flexibility.

I will likely put this into release 3.4.0 and see if we get any feedback.

@coveralls
Copy link

coveralls commented Sep 13, 2023

Coverage Status

coverage: 100.0%. remained the same when pulling f4bc4d3 on matthewhegarty:issue-1573-CharWidget-return-str into 65f0085 on django-import-export:main.

@matthewhegarty matthewhegarty merged commit 4272d3f into django-import-export:main Sep 13, 2023
13 checks passed
@matthewhegarty matthewhegarty deleted the issue-1573-CharWidget-return-str branch September 14, 2023 18:42
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.

Maybe CharWidget.clean() should always return a string?
3 participants