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.

Sign up for GitHub

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

Jump to bottom

Preserve comments when serializing/deserializing with toJSON and fromJSON. #983

Merged
merged 6 commits into from
Feb 12, 2018
Merged

Preserve comments when serializing/deserializing with toJSON and fromJSON. #983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed99b3c
Preserve comments when serializing/deserializing with toJSON and from…
gideongoodwin Jan 19, 2018
c4fe23e
Adding an option keepComments for all toJSON implementations.
gideongoodwin Feb 6, 2018
7ba9bf7
Fix code climate issues.
gideongoodwin Feb 6, 2018
ac36320
Fixing Field constructor type shuffling for comment.
gideongoodwin Feb 6, 2018
a5e4048
CodeClimate fix: missing semicolon.
gideongoodwin Feb 7, 2018
34d5327
Updating types, fixing some type comments.
gideongoodwin Feb 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 51 additions & 14 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,20 @@ export class Enum extends ReflectionObject {
* @param name Unique name within its namespace
* @param [values] Enum values as an object, by name
* @param [options] Declared options
* @param [comment] The comment for this enum
* @param [comments] The value comments for this enum
*/
constructor(name: string, values?: { [k: string]: number }, options?: { [k: string]: any });
constructor(name: string, values?: { [k: string]: number }, options?: { [k: string]: any }, comment?: string, comments?: { [k: string]: string });

/** Enum values by id. */
public valuesById: { [k: number]: string };

/** Enum values by name. */
public values: { [k: string]: number };

/** Enum comment text. */
public comment: (string|null);

/** Value comment texts, if any. */
public comments: { [k: string]: string };

Expand All @@ -182,9 +187,10 @@ export class Enum extends ReflectionObject {

/**
* Converts this enum to an enum descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Enum descriptor
*/
public toJSON(): IEnum;
public toJSON(toJSONOptions?: IToJSONOptions): IEnum;

/**
* Adds a value to this enum.
Expand Down Expand Up @@ -288,8 +294,9 @@ export class FieldBase extends ReflectionObject {
* @param [rule="optional"] Field rule
* @param [extend] Extended type if different from parent
* @param [options] Declared options
* @param [comment] Comment associated with this field
*/
constructor(name: string, id: number, type: string, rule?: (string|{ [k: string]: any }), extend?: (string|{ [k: string]: any }), options?: { [k: string]: any });
constructor(name: string, id: number, type: string, rule?: (string|{ [k: string]: any }), extend?: (string|{ [k: string]: any }), options?: { [k: string]: any }, comment?: string);

/** Field rule, if any. */
public rule?: string;
Expand Down Expand Up @@ -342,11 +349,15 @@ export class FieldBase extends ReflectionObject {
/** Sister-field within the declaring namespace if an extended field. */
public declaringField: (Field|null);

/** Comment for this field. */
public comment: (string|null);

/**
* Converts this field to a field descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Field descriptor
*/
public toJSON(): IField;
public toJSON(toJSONOptions?: IToJSONOptions): IField;

/**
* Resolves this field's type references.
Expand Down Expand Up @@ -445,8 +456,9 @@ export class MapField extends FieldBase {
* @param keyType Key type
* @param type Value type
* @param [options] Declared options
* @param [comment] Comment associated with this field
*/
constructor(name: string, id: number, keyType: string, type: string, options?: { [k: string]: any });
constructor(name: string, id: number, keyType: string, type: string, options?: { [k: string]: any }, comment?: string);

/** Key type. */
public keyType: string;
Expand All @@ -465,9 +477,10 @@ export class MapField extends FieldBase {

/**
* Converts this map field to a map field descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Map field descriptor
*/
public toJSON(): IMapField;
public toJSON(toJSONOptions?: IToJSONOptions): IMapField;

/**
* Map field decorator (TypeScript).
Expand Down Expand Up @@ -586,8 +599,9 @@ export class Method extends ReflectionObject {
* @param [requestStream] Whether the request is streamed
* @param [responseStream] Whether the response is streamed
* @param [options] Declared options
* @param [comment] The comment for this method
*/
constructor(name: string, type: (string|undefined), requestType: string, responseType: string, requestStream?: (boolean|{ [k: string]: any }), responseStream?: (boolean|{ [k: string]: any }), options?: { [k: string]: any });
constructor(name: string, type: (string|undefined), requestType: string, responseType: string, requestStream?: (boolean|{ [k: string]: any }), responseStream?: (boolean|{ [k: string]: any }), options?: { [k: string]: any }, comment?: string);

/** Method type. */
public type: string;
Expand All @@ -610,6 +624,9 @@ export class Method extends ReflectionObject {
/** Resolved response type. */
public resolvedResponseType: (Type|null);

/** Comment for this method */
public comment: (string|null);

/**
* Constructs a method from a method descriptor.
* @param name Method name
Expand All @@ -621,9 +638,10 @@ export class Method extends ReflectionObject {

/**
* Converts this method to a method descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Method descriptor
*/
public toJSON(): IMethod;
public toJSON(toJSONOptions?: IToJSONOptions): IMethod;
}

/** Method descriptor. */
Expand Down Expand Up @@ -670,9 +688,10 @@ export class Namespace extends NamespaceBase {
/**
* Converts an array of reflection objects to JSON.
* @param array Object array
* @param [toJSONOptions] JSON conversion options
* @returns JSON object or `undefined` when array is empty
*/
public static arrayToJSON(array: ReflectionObject[]): ({ [k: string]: any }|undefined);
public static arrayToJSON(array: ReflectionObject[], toJSONOptions?: IToJSONOptions): ({ [k: string]: any }|undefined);

/**
* Tests if the specified id is reserved.
Expand Down Expand Up @@ -702,9 +721,10 @@ export abstract class NamespaceBase extends ReflectionObject {

/**
* Converts this namespace to a namespace descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Namespace descriptor
*/
public toJSON(): INamespace;
public toJSON(toJSONOptions?: IToJSONOptions): INamespace;

/**
* Adds nested objects to this namespace from nested object descriptors.
Expand Down Expand Up @@ -921,15 +941,19 @@ export class OneOf extends ReflectionObject {
* @param name Oneof name
* @param [fieldNames] Field names
* @param [options] Declared options
* @param [comment] Comment associated with this field
*/
constructor(name: string, fieldNames?: (string[]|{ [k: string]: any }), options?: { [k: string]: any });
constructor(name: string, fieldNames?: (string[]|{ [k: string]: any }), options?: { [k: string]: any }, comment?: string);

/** Field names that belong to this oneof. */
public oneof: string[];

/** Fields that belong to this oneof as an array for iteration. */
public readonly fieldsArray: Field[];

/** Comment for this field. */
public comment: (string|null);

/**
* Constructs a oneof from a oneof descriptor.
* @param name Oneof name
Expand All @@ -941,9 +965,10 @@ export class OneOf extends ReflectionObject {

/**
* Converts this oneof to a oneof descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Oneof descriptor
*/
public toJSON(): IOneOf;
public toJSON(toJSONOptions?: IToJSONOptions): IOneOf;

/**
* Adds a field to this oneof and removes it from its current parent, if any.
Expand Down Expand Up @@ -1016,6 +1041,16 @@ export interface IParseOptions {

/** Keeps field casing instead of converting to camel case */
keepCase?: boolean;

/** Recognize double-slash comments in addition to doc-block comments. */
alternateCommentMode?: boolean;
}

/** Options modifying the behavior of JSON serialization. */
export interface IToJSONOptions {

/** Serializes comments. */
keepComments?: boolean;
}

/**
Expand Down Expand Up @@ -1345,9 +1380,10 @@ export class Service extends NamespaceBase {

/**
* Converts this service to a service descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Service descriptor
*/
public toJSON(): IService;
public toJSON(toJSONOptions?: IToJSONOptions): IService;

/** Methods of this service as an array for iteration. */
public readonly methodsArray: Method[];
Expand Down Expand Up @@ -1497,9 +1533,10 @@ export class Type extends NamespaceBase {

/**
* Converts this message type to a message type descriptor.
* @param [toJSONOptions] JSON conversion options
* @returns Message type descriptor
*/
public toJSON(): IType;
public toJSON(toJSONOptions?: IToJSONOptions): IType;

/**
* Adds a nested object to this type.
Expand Down
22 changes: 17 additions & 5 deletions src/enum.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ var Namespace = require("./namespace"),
* @param {string} name Unique name within its namespace
* @param {Object.<string,number>} [values] Enum values as an object, by name
* @param {Object.<string,*>} [options] Declared options
* @param {string} [comment] The comment for this enum
* @param {Object.<string,string>} [comments] The value comments for this enum
*/
function Enum(name, values, options) {
function Enum(name, values, options, comment, comments) {
ReflectionObject.call(this, name, options);

if (values && typeof values !== "object")
Expand All @@ -35,11 +37,17 @@ function Enum(name, values, options) {
*/
this.values = Object.create(this.valuesById); // toJSON, marker

/**
* Enum comment text.
* @type {string|null}
*/
this.comment = comment;

/**
* Value comment texts, if any.
* @type {Object.<string,string>}
*/
this.comments = {};
this.comments = comments || {};

/**
* Reserved ranges, if any.
Expand Down Expand Up @@ -72,20 +80,24 @@ function Enum(name, values, options) {
* @throws {TypeError} If arguments are invalid
*/
Enum.fromJSON = function fromJSON(name, json) {
var enm = new Enum(name, json.values, json.options);
var enm = new Enum(name, json.values, json.options, json.comment, json.comments);
enm.reserved = json.reserved;
return enm;
};

/**
* Converts this enum to an enum descriptor.
* @param {IToJSONOptions} [toJSONOptions] JSON conversion options
* @returns {IEnum} Enum descriptor
*/
Enum.prototype.toJSON = function toJSON() {
Enum.prototype.toJSON = function toJSON(toJSONOptions) {
var keepComments = toJSONOptions ? Boolean(toJSONOptions.keepComments) : false;
return util.toObject([
"options" , this.options,
"values" , this.values,
"reserved" , this.reserved && this.reserved.length ? this.reserved : undefined
"reserved" , this.reserved && this.reserved.length ? this.reserved : undefined,
"comment" , keepComments ? this.comment : undefined,
"comments" , keepComments ? this.comments : undefined
]);
};

Expand Down
20 changes: 16 additions & 4 deletions src/field.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var ruleRe = /^required|optional|repeated$/;
* @throws {TypeError} If arguments are invalid
*/
Field.fromJSON = function fromJSON(name, json) {
return new Field(name, json.id, json.type, json.rule, json.extend, json.options);
return new Field(name, json.id, json.type, json.rule, json.extend, json.options, json.comment);
};

/**
Expand All @@ -50,13 +50,16 @@ Field.fromJSON = function fromJSON(name, json) {
* @param {string|Object.<string,*>} [rule="optional"] Field rule
* @param {string|Object.<string,*>} [extend] Extended type if different from parent
* @param {Object.<string,*>} [options] Declared options
* @param {string} [comment] Comment associated with this field
*/
function Field(name, id, type, rule, extend, options) {
function Field(name, id, type, rule, extend, options, comment) {

if (util.isObject(rule)) {
comment = extend;
options = rule;
rule = extend = undefined;
} else if (util.isObject(extend)) {
comment = options;
options = extend;
extend = undefined;
}
Expand Down Expand Up @@ -183,6 +186,12 @@ function Field(name, id, type, rule, extend, options) {
* @private
*/
this._packed = null;

/**
* Comment for this field.
* @type {string|null}
*/
this.comment = comment;
}

/**
Expand Down Expand Up @@ -227,15 +236,18 @@ Field.prototype.setOption = function setOption(name, value, ifNotSet) {

/**
* Converts this field to a field descriptor.
* @param {IToJSONOptions} [toJSONOptions] JSON conversion options
* @returns {IField} Field descriptor
*/
Field.prototype.toJSON = function toJSON() {
Field.prototype.toJSON = function toJSON(toJSONOptions) {
var keepComments = toJSONOptions ? Boolean(toJSONOptions.keepComments) : false;
return util.toObject([
"rule" , this.rule !== "optional" && this.rule || undefined,
"type" , this.type,
"id" , this.id,
"extend" , this.extend,
"options" , this.options
"options" , this.options,
"comment" , keepComments ? this.comment : undefined
]);
};

Expand Down
14 changes: 9 additions & 5 deletions src/mapfield.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ var types = require("./types"),
* @param {string} keyType Key type
* @param {string} type Value type
* @param {Object.<string,*>} [options] Declared options
* @param {string} [comment] Comment associated with this field
*/
function MapField(name, id, keyType, type, options) {
Field.call(this, name, id, type, options);
function MapField(name, id, keyType, type, options, comment) {
Field.call(this, name, id, type, undefined, undefined, options, comment);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm, this looks like an old bug calling base class constructor with wrong params.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe omitting rule and extend and instead passing options should be handled here. Might well be, though, that specifying undefined explicitly is better optimizable by JS engines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn't see the parameter shuffling in the Field constructor...I think it's clearer to explicitly pass undefined values for these, what do you think? Since options can be undefined here as well, the duck typing in Field() gets a little nondeterministic if these values are omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added additional parameter shuffling to handle the comment parameter.


/* istanbul ignore if */
if (!util.isString(keyType))
Expand Down Expand Up @@ -64,20 +65,23 @@ function MapField(name, id, keyType, type, options) {
* @throws {TypeError} If arguments are invalid
*/
MapField.fromJSON = function fromJSON(name, json) {
return new MapField(name, json.id, json.keyType, json.type, json.options);
return new MapField(name, json.id, json.keyType, json.type, json.options, json.comment);
};

/**
* Converts this map field to a map field descriptor.
* @param {IToJSONOptions} [toJSONOptions] JSON conversion options
* @returns {IMapField} Map field descriptor
*/
MapField.prototype.toJSON = function toJSON() {
MapField.prototype.toJSON = function toJSON(toJSONOptions) {
var keepComments = toJSONOptions ? Boolean(toJSONOptions.keepComments) : false;
return util.toObject([
"keyType" , this.keyType,
"type" , this.type,
"id" , this.id,
"extend" , this.extend,
"options" , this.options
"options" , this.options,
"comment" , keepComments ? this.comment : undefined
]);
};

Expand Down