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

Fixed TimeField not handling string times. #3809

Merged
merged 2 commits into from Jan 11, 2016

Conversation

areski
Copy link
Contributor

@areski areski commented Jan 7, 2016

Similar to #3731 but for TimeField

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 7, 2016

I think with a test case it'll be ready to merge.

@xordoquy xordoquy added this to the 3.3.3 Release milestone Jan 7, 2016
@areski
Copy link
Contributor Author

areski commented Jan 8, 2016

@xordoquy here the test, I could not find it when I opened the PR but after more digging I think I found it.

@@ -1204,6 +1204,8 @@ def to_representation(self, value):
)

if output_format.lower() == ISO_8601:
if (isinstance(value, str)):
Copy link
Contributor

@m1kola m1kola Jan 10, 2016

Choose a reason for hiding this comment

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

Is it works ok on python 2.* with unicode values? Seems similar to #3819

Copy link
Collaborator

@xordoquy xordoquy Jan 11, 2016

Choose a reason for hiding this comment

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

Could you update str to six.string_types ? (as per #3819 indeed)

Copy link
Contributor Author

@areski areski Jan 11, 2016

Choose a reason for hiding this comment

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

Sure, I will then update both DateField & TimeField to use six.string_types

Copy link
Collaborator

@xordoquy xordoquy Jan 11, 2016

Choose a reason for hiding this comment

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

@areski The DateTime was out of this PR scope and now conflict with master. Could you merge it please ?

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 11, 2016

Thanks :)

@areski
Copy link
Contributor Author

areski commented Jan 11, 2016

Thanks for the heads-up :)

xordoquy added a commit that referenced this issue Jan 11, 2016
Fixed TimeField not handling empty values
@xordoquy xordoquy merged commit efe2c37 into encode:master Jan 11, 2016
2 checks passed
@xordoquy xordoquy changed the title Fixed TimeField not handling empty values Fixed TimeField not handling string times. Jan 11, 2016
@xordoquy
Copy link
Collaborator

xordoquy commented Jan 11, 2016

Thanks for the work !

@areski
Copy link
Contributor Author

areski commented Jan 11, 2016

Thanks for the guidance!

On Mon, Jan 11, 2016 at 12:29 PM, Xavier Ordoquy notifications@github.com
wrote:

Thanks for the work !


Reply to this email directly or view it on GitHub
#3809 (comment)
.

Kind regards,
/Areski


Arezqui Belaid, areski@gmail.com
Founder at Star2Billing (www.star2billing.com)

Tel: +34650784355
Twitter: http://twitter.com/areskib
LinkedIn: http://www.linkedin.com/in/areski

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants