-
Notifications
You must be signed in to change notification settings - Fork 1
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
PRO-2188 PRO-2213 PRO-2210 PRO-2208 Init #1
Conversation
@@ -0,0 +1,130 @@ | |||
// http://talesofthefluxfox.com/2016/10/07/writing-to-xlsx-spreadsheets-in-node-js/ | |||
|
|||
const ALPHA = [ 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied verbatim from A2
const assert = require('assert'); | ||
const testUtil = require('apostrophe/test-lib/test'); | ||
|
||
describe('Pieces Exporter', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add tests for tsv and excel before publishing
PRO-2188 As a developer, I can install and configure the Export module
This ticket is just to enable the support for installing and configuration the new export module. The actual export functionality is in subsequent tickets. The module should support configuration for specific document types. Acceptance criteria:
|
} | ||
``` | ||
|
||
#### Export areas as plain text with `exportPlainText` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apostrophe convention elsewhere is that plaintext is one word and therefore notCamelCased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So exportPlaintext
would be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plaintext seems to be an encryption term, not the opposite of rich text. I was going for readability and clarity here. I see we're using plaintext
in core, but it seems less of an intentional convention than something that was never examined (since richtext
isn't used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shit, you're right.
It's not unexamined, it's more like misexamined, time has been put into it in the past, I'm surprised I never found that dictionary definition and said "ruh roh, I'm totally off base here." But... I didn't.
So I guess you should go ahead and do it correctly here, but we're stuck with it in the current way in core for 3.x.
lib/export.js
Outdated
} | ||
}; | ||
|
||
async function writeBatch (req, out, lastId = '', reporting, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're away from callback hell, I'm inclined to suggest this method doesn't need to be recursive anymore. Then it can be true to its name and just return true if it should be called again, and be awaited in a loop. Easier to understand.
lib/export.js
Outdated
|
||
}); | ||
|
||
await self.writeBatch(req, out, '', reporting, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where that little while loop would go.
No description provided.