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 time format #276

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

yuemori
Copy link
Contributor

@yuemori yuemori commented Feb 22, 2016

Define date-time validation format with RFC3339 in JSON Schema.
I encounted problem, Prmd.combine return not valid format value.

from:

  created_at:
    description: when user was created
    format: date-time
    example: 2016-01-01T00:00:00Z
    type:
    - string

expect:

    "created_at": {
      "description": "when user was created",
      "format": "date-time",
      "example": "2016-01-01T00:00:00Z",
      "type": [
        "string"
      ]
    },

got:

    "created_at": {
      "description": "when user was created",
      "format": "date-time",
      "example": "2016-01-01 00:00:00 UTC",
      "type": [
        "string"
      ]
    },

I would like to merge this solution.
But, this solution abandoned under Ruby2.0 and a large range of influence cause use prepend.
If there is no problem to merge.

@geemus
Copy link
Member

geemus commented Feb 23, 2016

I'm not very familiar with prepend. Do you think we would be better off just checking the type and if it is time doing special handling or something? ie falling back to strftime instead of to_json/to_s? Might be safer.

@yuemori
Copy link
Contributor Author

yuemori commented Feb 25, 2016

Thanks reply @geemus !

I completely agree with you. Your idea be safer than my change, but that change must be tough.

I think to be not worth the effort and YAGNI.
Because this ploblem derived from the ruby YAML parser and this problem occurred to only example field.

I feel there's a high possibility occurred in development environment only to those ploblems.
And, there is also a possibility to cause some problems from prepend , but that is low possibility for similar reasons.

Please forgive me my lousy English.
I hope I can express my thoughts to you even just a little bit.

@geemus
Copy link
Member

geemus commented Feb 29, 2016

I see, thanks for clarifying. I think we managed to avoid this as we use quotes around the timestamps (so they are treated as strings instead of timestamps when YAML parses them). This could work-around the issue at least. I see now why doing it the way I described wouldn't work and I also have a hard time seeing how to do it more simply or without a patch like this. I'm still concerned about possible side-effects or other issues to this kind of change though, so for now could you just quote to work-around? Thanks!

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.

2 participants