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

[TIMOB-26025] TiAPI: Improve Ti.Filesystem.File parity #10019

Merged
merged 12 commits into from May 11, 2018

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented May 1, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-26025

Description:
This stole quite a bit of @hansemannn 's PR from #9750 on the iOS side to better define return types of methods/properties and at least log errors that may happen under the hood. This did not incorporate the ARC changes (I was concerned they were the cause of the crashes on that PR).

iOS:

  • Deprecate Ti.Filesystem.File #createTimestamp() and #modificationTimestamp() and the property equivalents (log deprecation note when old methods are called). Add #createdAt() and #modifiedAt() as new replacements that return a Date.
  • Add Ti.Filesystem.File#copy() method.
  • Default Ti.Filesystem.File#createDirectory() to be recursive like other platforms. This took an undocumented single argument to do this recursively in the past. I retained the ability to pass an explicit false in to the method to not create a directory recursively - not sure what we want to do here. This is a behavioral change.
  • Enable the Ti.Filesystem.File.parent test

Android:

  • Deprecate Ti.Filesystem.File #createTimestamp() and #modificationTimestamp() (log deprecation note when old methods are called). Add #createdAt() and #modifiedAt() as new replacements that return a Date.
  • Add Ti.Filesystem.File#append() method.
  • Fix returning true for Ti.Filesystem.File#isDirectory() on non-existent Resource sub-dirs.

JIRA:

…le#createDirectory to be recursive like other platforms (but allow passing false as arg to disable). Fix Android returning true for Ti.Filesystem.File#isDirectory() on non-existent Resource sub-dirs.
@sgtcoolguy
Copy link
Contributor Author

So my concern here is that introducing alternate methods would be a bit of work for all the platforms, and that the new names actually suggest return types of Date mores than the current names. iOS just happens to return Date objects for a method clearly named to suggest a unix timestamp return value. I think it makes sense to make a deprecation note now and not change the return type to Number yet, but I don't see a nice way to avoid the issue of the method suddenly changing return type on iOS in the future.

Moving to a new method name is one way to do it, as you suggested, but then that suggests moving all the other platforms to Date to me and requires more work - and I'd still be reluctant to remove the timestamp variants.

Looking at Node's fs API, they actually have both Date and Number versions (ctime vs ctimeMs, time vs mtimeMs)

So maybe we combine our solutions?

  • Introduce new createdAt and modifiedAt methods that return Date on all platforms
  • Add deprecation note on iOS's creationTimestamp and modificationTimestamp that they will return a Number in a future release (8.0? 9.0?) and to use createdAt or modifiedAt variants to get a Date object.

@Topener
Copy link
Sponsor Contributor

Topener commented May 8, 2018

Date Objects make a lot of sense in JavaScript world. For a lot of purposes you'd actually be making date objects yourself as well. And getting timestamp from date object is easy.

I'm in favour of just having 1 property, based on Hans his suggestion, the createdAt and modifiedAt properties. Removal in 9.0 makes sense too.

@hansemannn
Copy link
Collaborator

@sgtcoolguy I can help doing the createdAt / modifiedAt changes incl. the deprecation in a separate PR. Let me know!

@hansemannn hansemannn changed the title PARITY - Ti.Filesystem.File [TIMOB-26025] TiAPI: Improve Ti.Filesystem.File parity May 9, 2018
…turn Dates. Deprecate #modificationTimestamp() and #createTimestamp() due to inconsistent return types across platforms. Update unit tests to check new createdAt/modifiedAt methods. Add docs for new methods/deprecations. Use longs in place of doubles in Android file APIs whne it's more appropriate. Support getting creation timestamp for files on Android when API 26+. Fix spaceAvailable to use newer more accurate APIs on API level 18+.
@sgtcoolguy
Copy link
Contributor Author

sgtcoolguy commented May 9, 2018

@infosia Hey Kota - so to fix some parity issues we're looking at deprecating Ti.Filesystem.File #createTimestamp() and #modificationTimestamp() to be replaced by new #createdAt() and #modifiedAt() methods that return a Date.

@sgtcoolguy
Copy link
Contributor Author

@hansemannn @Topener I've pushed the changes to deprecate the timestamp methods on iOS and Android and added the new variants that return a Date.

@build
Copy link
Contributor

build commented May 9, 2018

Fails
🚫

Tests have failed, see below for more information.

Messages
📖

💾 Here's the generated SDK zipfile.

Tests:

Classname Name Time Error
android.Titanium.UI.ScrollableView moveX-scrollTo 20.044 Error: timeout of 20000ms exceeded

Generated by 🚫 dangerJS

@sgtcoolguy sgtcoolguy merged commit 4e4abc8 into tidev:master May 11, 2018
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
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

4 participants