Skip to content
This repository has been archived by the owner on Mar 16, 2019. It is now read-only.

Fix for issue #468, #461, #460 and minor cleanup #469

Merged
merged 7 commits into from
Aug 9, 2017
Merged

Fix for issue #468, #461, #460 and minor cleanup #469

merged 7 commits into from
Aug 9, 2017

Conversation

lll000111
Copy link
Contributor

@lll000111 lll000111 commented Aug 8, 2017

Fixes #468, #461, #460 (partially)

Thank you for making a pull request ! Just a gentle reminder :)

  1. If the PR is offering a feature please make the request to our "Feature Branch" 0.11.0
  2. Bug fix request to "Bug Fix Branch" 0.10.9 <--
  3. Correct README.md can directly to master

bcpclone and others added 6 commits August 3, 2017 09:37
- fix minor errors in JSDoc comments, for example {string]} => {string}
- fix parameter name "encode" => "encoding" (more logical, and it says so in the function's JSDoc too)
- json-stream.js: split a looooong log message string constant into two parts and fix a typo ("maually"), and the type for objects is "Object" (capitalized) in Flow type annotations
@lll000111 lll000111 changed the title Fix for issue #468 Fix for issue #468, #461, and minor cleanup Aug 8, 2017
…lization)

Error messages reported by iOS and Android versions should be as similar as possible. Also, within the same system there should be consistency. This patch is an attempt to bring a LITTLE more of this consistency to the error messages. I also fixed some very few minor language issues, like "does not exist" (is the correct English). I tried keeping the changes to a minimum.

Background: In my project code I want to know when a file already exists (e.g. after calling fs.createFile), and the only way is to check the error message string that I get. It's bad if they differ between versions (createFileASCII and createFile) and then also between Android and iOS version. At least some core part of the string should be the same, so that I have something to match.

Ideally messages should come from a centralized easy (easier) to maintain file (for both iOS and Android), and ideally both systems should have the same errors and messages as far as possible.
@lll000111 lll000111 changed the title Fix for issue #468, #461, and minor cleanup Fix for issue #468, #461, #460 and minor cleanup Aug 8, 2017
@wkh237
Copy link
Owner

wkh237 commented Aug 9, 2017

Thanks for the great PR !

@wkh237 wkh237 merged commit b634402 into wkh237:0.10.9 Aug 9, 2017
@lll000111
Copy link
Contributor Author

lll000111 commented Aug 9, 2017

@wkh237 Unfortunately I woke up at 4 am and thought to myself - I missed the fact that some methods use ]React Native's Promise methods](https://facebook.github.io/react-native/docs/native-modules-ios.html#promises) to return a rejected promise directly from the low-level code (while others return a string via callback). I don't see in the documentation (link above) if that promise rejection already results in an object? In those cases an additional new Error(err) should be reverted

CORRECTION: I see new Error is only added for the callback-based errors, and I expect the promise returns an Error object simply because it has a "code" and a "message", and that results in only one parameter coming out.

Also, the error message changes may require a WARNING to users: Other people may have hard-coded the error strings into their code in "if" statements. It's still worth it I dare say.

danielsuo added a commit to danielsuo/react-native-fetch-blob that referenced this pull request Feb 13, 2018
* upstream/0.10.9:
  Fixed problem with type casting (wkh237#513)
  My proposed 0.10.9 changes (wkh237#489)
  wkh237#268 Cancelled task should not trigger `then` promise function
  Add ability to cancel android DownloadManager fetches (wkh237#502)
  Fix iOS initialization race condition (wkh237#499)
  prevent UIApplication methods from being called on background thread (wkh237#486)
  Implemenet fs.hash() -- wkh237#439 "Feature: Calculate file hash" (wkh237#476)
  I forgot one error string for the Android readFile() method (wkh237#475)
  Fix for wkh237#467 (wkh237#472)
  Fix for issue wkh237#468, wkh237#461, wkh237#460 and minor cleanup (wkh237#469)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants