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

Patch to implement feature, exporting notes to JSON (#912, issues/912). #927

Merged
merged 3 commits into from Nov 11, 2018

Conversation

Projects
None yet
2 participants
@moltenjs
Contributor

moltenjs commented Oct 28, 2018

Implementing for #912.

}
async processItem(ItemClass, item) {
const obj = await this.buildPlainObjectForJson_(ItemClass, item);

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

I would have expected that simply calling JSON.stringfy(item) woudl be enough. Any reason this buildPlainObjectFromJson function is needed? Are there keys in item that should not be serialized? Note that we also need to export type_ or else it's not possible to know the object type.

This comment has been minimized.

@moltenjs

moltenjs Nov 3, 2018

Contributor

stringify(item) could work, but I think it's less fragile to have more control over what is written to disk, in case future code ever changes the structure of 'item'. I can simplify the method a bit though by creating a simpler object. Yes, the export does include type_, and it's verified by the test.

This comment has been minimized.

@laurent22

laurent22 Nov 7, 2018

Owner

stringify(item) could work, but I think it's less fragile to have more control over what is written to disk, in case future code ever changes the structure of 'item'.

I see what you mean and in some cases it would make sense to follow your approach. However in this case, the structure of the items is quite fixed - there might be new properties added but that will need to be serialised too, and on the other hand there won't be anything unusual added to it or any special properties (like type_) so it's safe to simply serialise it with JSON.stringfy and save it to disk. The item is basically exactly a row in the database table, except for the type_ property.

async processItem(ItemClass, item) {
const obj = await this.buildPlainObjectForJson_(ItemClass, item);
const fileName = this.getNameWithDifferentExtension_(ItemClass, item, ".json");

This comment has been minimized.

@laurent22

laurent22 Nov 2, 2018

Owner

Instead of having a specialised function here, I think it would be better to change the systemPath method to this:

	static systemPath(itemOrId, extension = null) {
		if (extension === null) extension = 'md'
		if (typeof itemOrId === 'string') return itemOrId + '.' + extension;
		return itemOrId.id + '.' + extension;
	}

Then here you can simply do const filename = ItemClass.systemPath(item, 'json');

This comment has been minimized.

@moltenjs

moltenjs Nov 3, 2018

Contributor

Agreed, done.

Ben Fisher added some commits Nov 3, 2018

@moltenjs

This comment has been minimized.

Contributor

moltenjs commented Nov 8, 2018

Submitted new changes. I see that "AppVeyor build failed", but don't think it is related to my changes, I haven't touched any package.json files.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Nov 11, 2018

All looks good, thanks a lot @downpoured!

@laurent22 laurent22 merged commit 0eb18d2 into laurent22:master Nov 11, 2018

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment