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

Refactor and Modernize ISO8601DateFormatter #21

Open
2 of 8 tasks
blakewatters opened this issue Sep 3, 2013 · 6 comments
Open
2 of 8 tasks

Refactor and Modernize ISO8601DateFormatter #21

blakewatters opened this issue Sep 3, 2013 · 6 comments

Comments

@blakewatters
Copy link

Hi Peter -

Thanks so much for your most excellent contribution of ISO8601DateFormatter to the Cocoa Open Source community. It definitely has to be one of the most widely used libraries floating around.

Several years ago I integrated the library into RestKit/RestKit and have been maintaining a forked version of the library ever since. We have made a number of incremental improvements to the package. We are currently in the process of atomizing some parts of RestKit into smaller more modular libraries and one of the sticking points at the moment is the dependence on the forked copy of the date formatter.

This has put me in the position of either a) shipping a standalone competing implementation of the date formatter or b) trying to integrate our changes back into the mainline so that there is one authoritative code base out there. I'd prefer to go the latter route and wanted to reach out and gauge your enthusiasm for larger changes to the library.

I'd like to do the following:

  • Put an Xcode project on tree and integrate unit tests that aren't dependent on python / shell scripting.
  • Put the project under Travis CI for continuous integration of the tests.
  • Modernize the date formatter by updating it for ARC.
  • Integrate whatever fixes I have in my codebase that have not appeared in your main line and add appropriate test coverage.
  • Clean up the formatting and styling in use within the library to be more in line with modern idiomatic Cocoa.
  • Improve the thread-safety of the library by factoring the parsing state out of the main class such that a new parsing operation object is created on each invocation.
  • Refactor the parsing routines to be simpler to read and follow. The library has excellent support for the ISO date format standard, but is rather hard to follow.
  • Mark up the source code with Appledoc

Let me know if this is of interest to you and you'd be willing to merge such changes. I am also happy to help you with on-going maintenance of the library.

@boredzo
Copy link
Owner

boredzo commented Sep 3, 2013

  • Xcode project: Already done.
  • Non-shell/-Python-based unit tests: Already begun, though many more of these tests are needed before the test monsters can be killed off.

Most other refactoring is blocked (AFAIC) on having much more test coverage in the modern tests.

  • ARC: Blocked on tests. At least one patch already exists; see the Network page. (Then again, I will probably redo this using whenever-current Xcode, both because of the various fixes that I've merged in and in order to pick up any and all new modernizations that have been introduced since the previous ARCifications.)
  • Formatting/style clean-up: Not sure what you mean.
  • “Parsing operation” object: Would likely destroy performance in non-thread-contended cases; reuse of the same calendar, date formatter, etc. is a tremendous performance win (see 0.6 release notes in readme). Not sure there's a good way around this. (I wonder if copying the objects would help…)
  • Refactoring for readability: Definitely needed. The current code predates NSRegularExpression. Much of it could probably be replaced with a regex or two or three.

@boredzo
Copy link
Owner

boredzo commented Sep 13, 2013

As I look at how I'll eventually refactor the parser, I'm starting to think that some sort of parsing operation object might be a good idea. It won't help with concurrency, but breaking the Single Giant Method into multiple methods will help with readability and redundancy.

@boredzo
Copy link
Owner

boredzo commented Sep 13, 2013

Setup of the parser might look something like this:

@synchronized(self) {
    ISO8601Parser *parser = [[ISO8601Parser alloc] initWithCalendar:… /*and whatever else it'll need*/];
    //Possibly more setup here
}
NSDateComponents *components = [parser parseString:string timeZone:&timeZone range:&range fractionOfSecond:&fractionOfSecond];

If that's enough (this would need to be VERY thoroughly tested), maybe this will help with concurrency after all.

At any rate, all such rewrites remain on hold until the test coverage of ISO8601DateFormatter.m gets sufficiently close to 100% and all old tests are ported and deleted—see #23. I at least want the coverage indicator on Coveralls to turn green.

@blakewatters
Copy link
Author

Cool I will jump back in on this shortly. I have an ARC'ified and patched version of the date formatter that was in RestKit that is now spun out into an external project (see https://github.com/blakewatters/ISO8601DateFormatterValueTransformer/tree/master/Code). I'll need to review the history of my changes and get a pull request over so I can drop the fork.

I have some test cases and regular expression work that can also be brought upstream. I'll try and carve out some time this weekend to get these bits brought over.

@boredzo boredzo added this to the 1.0 milestone Apr 17, 2016
@boredzo
Copy link
Owner

boredzo commented Apr 17, 2016

1.0 is going to be the “modernize all the things” release—more specifically, it's going to be exactly equivalent to 0.9, but less compatible.

#66 is another subtask of this.

@boredzo
Copy link
Owner

boredzo commented Apr 17, 2016

Also includes #69 (and I'm going to start using a label for all these subtasks).

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

No branches or pull requests

2 participants