Skip to content

Methods for ActiveSupport::Duration parsing from ISO 8601 and output to it #16917

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

Closed
wants to merge 21 commits into from
Closed

Methods for ActiveSupport::Duration parsing from ISO 8601 and output to it #16917

wants to merge 21 commits into from

Conversation

Envek
Copy link
Contributor

@Envek Envek commented Sep 14, 2014

This PR add methods to ActiveSupport::Duration to allow present it in ISO 8601 Duration format and instantiate from it.

ISO 8601 Duration format is standartized way to represent duration for interchange, it's already recognized by some database engines (e.g. PostgreSQL) and client side libraries (e.g. plugin for Moment.js durations). So, I think it should be part of ActiveSupport::Duration.

Some parts of code and tests are taken from ISO8601 gem by Arnau Siches (@arnau) and contributors. Many thanks to them.

This PR is required for PostgreSQL interval datatype support (for converting it from and to ActiveSupport::Duration) as I've proposed in Google Group here. See #16919. Because of that my parsing allows individual datetime parts to be negative (see PostgreSQL interval output).

There is no backward incompatible changes as I want it to be included in upcoming 4.2 release, if it still possible (pleeeaasee!).

end
end

def test_iso8601_parsing_valid_patterns
Copy link
Contributor

Choose a reason for hiding this comment

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

does this test just repeat part of what's already tested in test_iso8601_output_and_reparsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it repeat. This test only checks that validations doesn't complain. test_iso8601_output_and_reparsing tries to test some logic (as possible with weird ActiveSupport::Duration).
Should I remove this test case?

arnau added a commit to arnau/ISO8601 that referenced this pull request Sep 14, 2014
Thanks to @egilburg comment in rails/rails#16917 pull
request for catching this.
arnau added a commit to arnau/ISO8601 that referenced this pull request Sep 14, 2014
# Initialize new duration
time = ::Time.now
new(time.advance(parts) - time, parts)
end
Copy link
Member

Choose a reason for hiding this comment

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

There's enough going on here to extract a separate ISO8601DurationParser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where it should be placed? Same file (inside Duration) or separate file? Its location?

@Envek
Copy link
Contributor Author

Envek commented Sep 15, 2014

Fixed some issues:

  • extracted parsing in separate ActiveSupport::Duration::ISO8601DurationParser class in separate file
  • extracted parts normalization into separate method
  • fixed parts normalization so returned hash default value is nil (as usual)

Method parse! throws exception ActiveSupport::Duration::ISO8601DurationParser::ParsingError on invalid input while parse returns nil.

Rebased branch on top of current master due to changes in ActiveSupport::Duration in recently merged #16574 .

@Envek
Copy link
Contributor Author

Envek commented Sep 16, 2014

Fixed some more remarks.

I've noticed that now there is next error occurs when I run activerecord's tests for #16919 on top of this branch:

/Users/anovikov/rails/activesupport/lib/active_support/duration/iso8601_duration_parser.rb:2: warning: loading in progress, circular require considered harmful - /Users/anovikov/rails/activesupport/lib/active_support/duration.rb
    from /Users/anovikov/.rvm/gems/ruby-2.1.2-gost/gems/rake-10.3.2/lib/rake/rake_test_loader.rb:4:in  `<main>'
    from /Users/anovikov/.rvm/gems/ruby-2.1.2-gost/gems/rake-10.3.2/lib/rake/rake_test_loader.rb:4:in  `select'
    from /Users/anovikov/.rvm/gems/ruby-2.1.2-gost/gems/rake-10.3.2/lib/rake/rake_test_loader.rb:15:in  `block in <main>'
    from /Users/anovikov/.rvm/gems/ruby-2.1.2-gost/gems/rake-10.3.2/lib/rake/rake_test_loader.rb:15:in  `require'
    from /Users/anovikov/rails/activerecord/test/cases/adapter_test.rb:1:in  `<top (required)>'
    from /Users/anovikov/rails/activerecord/test/cases/adapter_test.rb:1:in  `require'
    from /Users/anovikov/rails/activerecord/test/cases/helper.rb:15:in  `<top (required)>'
    from /Users/anovikov/rails/activesupport/lib/active_support/dependencies.rb:248:in  `require'
...
    from /Users/anovikov/rails/activesupport/lib/active_support/dependencies.rb:248:in  `block in require'
    from /Users/anovikov/rails/activesupport/lib/active_support/dependencies.rb:248:in  `require'
    from /Users/anovikov/rails/activesupport/lib/active_support/duration/iso8601_duration_parser.rb:1:in  `<top (required)>'
    from /Users/anovikov/rails/activesupport/lib/active_support/duration/iso8601_duration_parser.rb:2:in  `<module:ActiveSupport>'

How can I fix it? The reason seems to be in that I'm explicitly requires file with parser in duration.rb

@Envek
Copy link
Contributor Author

Envek commented Feb 8, 2015

To avoid circular require warnings (absolutely don't understand why are they appearing such a lot) included ISO8601 parser into file activesupport/lib/active_support/duration.rb. Squashed and rebased on top of current master. Please review one more time.

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

Did you require duration in the parser and vice versa? That sounds like it could cause the circular requires.

@Envek
Copy link
Contributor Author

Envek commented Feb 8, 2015

No, I had required parser only from duration (no requires from file with parser at all): commit that caused a lot of warnings.

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

Duration was being autoloaded. It hit class Duration in the parser which would trigger another autoload and thus the files would keep trying to load each other.

raise ParsingError.new("Invalid ISO 8601 duration: #{iso8601duration} (only last part can be fractional)")
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

✂️ this line

@fxn
Copy link
Member

fxn commented Feb 8, 2015

@kaspth the contrib app certainly understands a trailing [...] with authors separated by commas and other separators. See for example 6f8d9bd.

I didn't know GitHub had that feature... you sure it does?

@Envek
Copy link
Contributor Author

Envek commented Feb 8, 2015

I've accounted all the notes and force pushed changes in the same commit. I've tried split initializer on separate methods, but dislike current implementations (they are linked too tight with each other).

P.S> in 6f8d9bd only one author specified. Git doesn't support multiple authors (at least with each one's email)

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2015

@fxn I did remember seeing commits like that. I assumed it was for GitHub and not the contributors app, but it was the other way. Sorry about that!

@Envek
Copy link
Contributor Author

Envek commented Feb 8, 2015

Ah, sorry, misunderstood you. Added both authors in [] in end of commit message.

@fxn
Copy link
Member

fxn commented Feb 8, 2015

@Envek If the body of the commit message ends with

[Andrey Novikov, Arnau Siches]

at the very bottom, last line, the contrib app will pick it up automatically and give both of you credit.

See for example 84c0f73. That one uses "&" but a comma also works.

parse!(iso8601duration)
rescue ISO8601Parser::ParsingError
nil
end
Copy link
Member

Choose a reason for hiding this comment

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

IMO we don't need a method that swallows the parse error.

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

If we're pulling in code under MIT license, we need to include the license and copyright statement.

Also, if we're pulling in code, perhaps we should do a gem dep instead?

Nice work on this, and patience 😁

@Envek
Copy link
Contributor Author

Envek commented Apr 17, 2016

@jeremy there is not a much code left from the original code from ISO8601 gem as I've rewritten parser from regexp to string scanner by @pixeltrix suggestion. As far as I can see for now only duration examples strings inside tests are same with ISO8601. See the original code and tests.

Anyway where I should place license and copyright statement if I should? @arnau what do you think?

Gem dependency means that we should throw away whole ActiveSupport::Duration and replace it with ISO8601::Duration. Are you serious with that? 😄

@jeremy please take a look at #22806 while I'm fixing your notes— it's extracted from this pull request.

@Envek
Copy link
Contributor Author

Envek commented Apr 17, 2016

@jeremy fixed your notes

@jeremy
Copy link
Member

jeremy commented Apr 17, 2016

@Envek We can't replace AS::Duration with ISO8601::Duration, but perhaps we could delegate implementation to it. You tell me :)

If you've rewritten the code, then you're no longer using the original code and aren't redistributing it, so you needn't pull in its license.

If you are reusing code, include the license+copyright along with the code, in a Ruby comment.

@arnau
Copy link

arnau commented Apr 18, 2016

@Envek do whatever makes more sense to you :)

@Envek
Copy link
Contributor Author

Envek commented Apr 18, 2016

@jeremy, @arnau, I changed notices about licensing, take a look at 3a92929 . Is it enough?

If yes I would be glad to rebase and squash.

@@ -224,6 +224,9 @@ def test_comparable
assert_equal(1, (61 <=> 1.minute))
end

# ISO8601 string examples are taken from ISO8601 gem at https://github.com/arnau/ISO8601/blob/b93d466840/spec/iso8601/duration_spec.rb
# published under the conditions of MIT license: https://github.com/arnau/ISO8601/blob/b93d466840/LICENSE

Copy link
Member

Choose a reason for hiding this comment

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

The license is pretty clear about what we need to include here:

The above copyright notice and this permission notice shall be
included in all copies or substantial portions of the Software.

So we need to include the license text in a comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included the license text in a comment

@@ -130,6 +133,23 @@ def respond_to_missing?(method, include_private=false) #:nodoc:
@value.respond_to?(method, include_private)
end

# Creates a new Duration from string formatted according to ISO 8601 Duration.
#
# See http://en.wikipedia.org/wiki/ISO_8601#Durations
Copy link
Member

Choose a reason for hiding this comment

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

See http://en.wikipedia.org/wiki/ISO_8601#Durations .

class Duration
# Parses a string formatted according to ISO 8601 Duration into the hash.
#
# See [ISO 8601](http://en.wikipedia.org/wiki/ISO_8601#Durations) for more information.
Copy link
Member

Choose a reason for hiding this comment

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

😢 Sorry this is rdoc, my bad. Let me see.

Copy link
Member

Choose a reason for hiding this comment

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

So rdoc makes sure the link is proper even if it's followed by a '.' Anyway, lets change to -

See {ISO 8601}[http://en.wikipedia.org/wiki/ISO_8601#Durations] for more information.

@jeremy
Copy link
Member

jeremy commented Apr 18, 2016

Merged! 04c512d

@jeremy jeremy closed this Apr 18, 2016
vipulnsward added a commit to vipulnsward/rails that referenced this pull request Apr 18, 2016
spastorino added a commit that referenced this pull request Apr 19, 2016
@Envek
Copy link
Contributor Author

Envek commented Apr 19, 2016

@jeremy thank you for merging! You have just unlocked a special #16919!

P.S> I have a one more small ActiveSupport goodness: #20625, you can review it too if you have free time and wish :-)

@Envek Envek deleted the iso8601_duration branch April 19, 2016 18:55
chancancode added a commit that referenced this pull request Jun 27, 2016
@ghost ghost deleted a comment Jul 16, 2019
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.