Add support for localized TimeZone names #46
Conversation
| @@ -105,7 +105,7 @@ def copyAndReplace(srcDirs: Seq[File], destinationDir: File): Seq[File] = { | |||
| // Copy the source files from the base project, exclude classes on java.util and dirs | |||
| val generatedFiles: List[java.io.File] = onlyScalaDirs.foldLeft(Set.empty[File]) { (files, sourceDir) => | |||
| files ++ copyDirectory(sourceDir, destinationDir, overwrite = true) | |||
| }.filterNot(_.isDirectory).filterNot(_.getParentFile.getName == "util").toList | |||
| }.filterNot(_.isDirectory).filter(_.getName.endsWith(".scala")).filterNot(_.getParentFile.getName == "util").toList | |||
There was a problem hiding this comment.
Is this change to avoid translating the .DS_store and other binary files?
There was a problem hiding this comment.
yes, this is exactly for that. It could also be achieved in the copyDirectory method by passing a filter that only takes .scala files. Something in the likes of PathFinder(source).**(scalaFileFilter) but that may make the build skip copying resources that could be needed afterwards, so I thought that skipping non-scala files before doing the replaceAll could suffice.
No problem of rolling this back if you think that the build should not take care of it...
There was a problem hiding this comment.
This is fine just wanted to make sure, this is probably faster anyway
| def getAvailableIDs(offsetMillis: Int): Array[String] = ??? | ||
| def getAvailableIDs: Array[String] = ZoneRulesProvider.getAvailableZoneIds.asScala.toArray | ||
| def getAvailableIDs(offsetMillis: Int): Array[String] = | ||
| getAvailableIDs.view.map(getTimeZone).filter(_.getRawOffset == offsetMillis).map(_.getID).toArray |
There was a problem hiding this comment.
why the view and toArray Couldn't you transform the array directly?
There was a problem hiding this comment.
The purpose of the view here is to fuse the map(...).filter(...).map(...) in one single traverse instead of three... but yeah, I could operate in the array straight.
There was a problem hiding this comment.
I'm not sure we use view in other places and a side effect is that we'll increase the binary size by importing the view classes.
At some moment after 2.0 I want to find out these cases to reduce the size of the export
There was a problem hiding this comment.
right, that makes sense. At this point this is just a convenience as the same can be achieved with a single operation.
| ??? | ||
|
|
||
| val id = getID | ||
| val zoneName = DateFormatSymbols.getInstance(locale).getZoneStrings.find(_(0) == id).flatMap { strs => |
There was a problem hiding this comment.
This is nice. I don't think we have this data en scala-java-locales but if we had it it would be good to get it
There was a problem hiding this comment.
The call _(0) could raise an exception. Could you handle it, e.g. filtering out empty arrays?
|
Thanks for this. It is starting to look good. I'm still trying to reproduce the issue with the standard test suite but extra tests are welcome too |
|
Thanks, will try to add some tests around this to have something more complete. |
|
Addressed previous comments and added a few tests. As you mentioned before, there is no locale information for time zones so the test in which I'm asserting over some of the display names fails ( |
|
I probably won't have time to add the names for the timezones to |
|
|
||
| val id = getID | ||
| def currentIdStrings(strs: Array[String]): Boolean = | ||
| atIndex(strs, 0).contains(id) |
There was a problem hiding this comment.
This is failing on travis. It seems contains is not supported on 2.10. Could you change this to e.g. exists?
|
It looks very nice indeed. Have you tested it in your application? |
|
Sorry for that 2.10 compile error, forgot to cross-compile it. The This is using a custom formatting pattern ( So, in essence this can be considered as fixed. I believe the build will still not pass due to test failures so if you prefer to delegate the missing bits towards the locales project I can remove/comment out the failing tests and consider this bit of work as complete. |
|
Thanks a lot for your contribution. Could you rebase on top of the latest master? The failing tests seems to be due to the missing locales data. Could you mark it as ignored or pending? It is certainly worth having it here |
|
Rebased on top of master and ignoring the display name test for now. This should be ready for merging after a successful build. |
alonsodomin
changed the title
|
Thanks a lot this seems ready. |
Fixes #44. Just a work in progress submission for enabling code review and discussion. The changes are focused on just the minimal effort needed to have #44 fixed and could benefit from some optimisations (i.e. some caching around the time zone localized names).
The small change in the
build.sbtfile is to ignore.DS_Storefiles that Macs usually create in folders when you navigate them, that was making compilation break when generating thejava.timeclasses since thereplaceAllfunctions where invoked in a binary file.