Issue866 #874

Merged
merged 10 commits into from May 15, 2012

2 participants

@mikechambers

All public FileSystem APIs now use IANA names (specified by w3c File System draft specification).

Created two new objects with constants:

NativeFileSystem.Encodings : contains IANA file encoding names, for use with the public file api.
NativeFileSystem_FSEncodings : contains encoding names used for internal, low level file system access (currently compatible with Node.js File API).

Updated all test cases to use Encoding constants.

Added two new test cases to test that public APIs take encoding names regardless of case.

Addressed Issue #866

mikechambers added some commits May 11, 2012
@mikechambers mikechambers Refactoring how file encodings are specified. Public API now takes IA…
…NA encoding names (case sensitive). Added constants for supported encoding types. Added internal API for converting between IANA encoding strings and node encoding strings. Removed all instances of hardcoded encoding strings 'utf8'
d2c0471
@mikechambers mikechambers Some refactoring. Now include constants for both IANA encoding string…
…s, as well as internal fs encoding strings. IANA encoding strings are case insensitive.
8d6bafa
@mikechambers mikechambers fixed tests that broke from previous commit. They were using wrong en…
…coding string for public file APIs.
a4d91d6
@mikechambers mikechambers updating test to use internal constants for file encodings 0540326
@mikechambers mikechambers updating test to use internal constants for file encodings d99973f
@mikechambers mikechambers updating test to use internal constants for file encodings 04e1954
@mikechambers mikechambers adding two new tests to check that encoding strings for readAsText wo…
…rk regardless of case. Minor refactoring of read file test code to allow reuse for multiple encodings.
1862071
@mikechambers mikechambers adding some comments 550961d
@mikechambers mikechambers replacing encoding string with constant 8f87209
@mikechambers

The pull request also adds an:

NativeFileSystem.Encodings._IANAToFS method

which converts file encoding strings between IANA strings and internal file encoding strings.

@redmunds redmunds was assigned May 15, 2012
@redmunds redmunds and 1 other commented on an outdated diff May 15, 2012
src/file/FileUtils.js
@@ -35,6 +35,8 @@ define(function (require, exports, module) {
Dialogs = require("widgets/Dialogs"),
Strings = require("strings");
+ var Encodings = NativeFileSystem.Encodings;

Code style nit: why start a new var? Seems more consistent to add to the list of vars above.

I did a separate one because it was a new require. I can move it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@redmunds redmunds and 1 other commented on an outdated diff May 15, 2012
src/file/NativeFileSystem.js
+ /** class: Encodings
+ *
+ * Static class that contains constants for file
+ * encoding types.
+ */
+ NativeFileSystem.Encodings = {};
+ NativeFileSystem.Encodings.UTF8 = "UTF-8";
+ NativeFileSystem.Encodings.UTF16 = "UTF-16";
+
+ /** class: _FSEncodings
+ *
+ * Internal static class that contains constants for file
+ * encoding types to be used by internal file system
+ * implimentation.
+ */
+ NativeFileSystem._FSEncodings = {};

Indent is incorrect. Did you run JSLint? I think it would have caught that.

Yeah. I did and it didnt give me any error, but ill double check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@redmunds redmunds and 1 other commented on an outdated diff May 15, 2012
src/file/NativeFileSystem.js
*/
NativeFileSystem.FileReader.prototype.readAsText = function (blob, encoding) {
var self = this;
if (!encoding) {
- encoding = "utf8";
+ encoding = _FSEncodings.UTF8;

Passing _FSEncodings.UTF8 to Encodings._IANAToFS() will cause it to return undefined, so this should be Encodings.UTF8, correct?

Or, you could set internalEncoding (see next comment) directly, and only call Encodings._IANAToFS() in the else condition.

Good catch... fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@redmunds redmunds and 1 other commented on an outdated diff May 15, 2012
src/file/NativeFileSystem.js
}
+
+ encoding = Encodings._IANAToFS(encoding);

I think that the iana versus internal/nodeJS encoding conversion going on here will be more clear if a new variable (e.g. "internalEncoding") is used instead of reusing the parameter that's passed in. This should also make debugging easier.

Good call. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@redmunds redmunds commented on an outdated diff May 15, 2012
test/spec/DocumentCommandHandlers-test.js
@@ -35,6 +35,10 @@ define(function (require, exports, module) {
DocumentManager, // loaded from brackets.test
SpecRunnerUtils = require("./SpecRunnerUtils.js");
+ var NativeFileSystem = require("file/NativeFileSystem").NativeFileSystem;

These vars should be added to var statement above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@redmunds

Done with initial review.

@mikechambers

New commit pushed. Addresses all comments. All tests pass.

@redmunds redmunds merged commit de7cc48 into adobe:master May 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment