Convert to classes and add suitenames attributes #61
Conversation
|
Brilliant, thanks! I'll try to get the review done as soon as possible. |
|
@davidparsson sorry to bug, I've had an issue with one of our CI reporters that this would let me fix nicely this week. I was wondering if you'll have time to get to review this PR soon or I should put a temporary hack into our CI for now to workaround it. (No pressure, I can totally workaround it but I'd like to fix it nicely if possible) |
|
@SimeonC, sorry for the delay. I'll get it done! |
These should always be overloaded.
| } | ||
| } | ||
|
|
||
| module.exports = JUnitReportBuilder; | ||
| module.exports = { Builder: JUnitReportBuilder }; |
There was a problem hiding this comment.
Are changes like this one completely backwards-compatible, or do they require adding curly brackets to the import statements (like this var { Builder } = require('./builder');)?
If they break backwards-compatibility, are they necessary? Best practice?
It might maybe be safe to assume that everyone uses (and can continue to use) this as the only import?
var builder = require('junit-report-builder');There was a problem hiding this comment.
Outside of src/index.js this won't have any affect as require('junit-report-builder') get's the default export of that file, not these files I refactored, because it resolves to the main field in package.json.
Technically they are breaking changes if someone does require('junit-report-builder/src/builder') then it will break, but technically we're not supposed to do that with node_modules 😂.
That said, this is a habit of mine - doing exports like this makes sure that we use the same name everywhere or explicitly rename it.
| /** | ||
| * @param {string} [message] | ||
| * @param {string} [type] | ||
| * @returns {TestCase} | ||
| * @chainable | ||
| */ |
There was a problem hiding this comment.
According to what I've found, @chainable is part of JSDoc but not part of TypeScript's supported JSDoc types. What are your thoughts on that? Is that intentional? Do they still add any value?
There was a problem hiding this comment.
I can't find the Stack Overflow/Github issue where I found that. Without it I think I did have more TS errors than with it so I think it might be something undocumented, or it was caused by something else and we don't actually need it anymore.
| /** | ||
| * @param {string} name | ||
| * @param {string} value | ||
| * @returns {TestNode} | ||
| * @chainable | ||
| */ |
There was a problem hiding this comment.
The type of value might be stricter than before, right? But requiring strings makes sense since we don't do any type conversions.
There was a problem hiding this comment.
Yea, technically the type is stricter as all the types used to be any. It won't break any existing code as you said we don't convert/check the values but it will throw TS errors on upgrading if the user is passing a number/boolean.
| @@ -1,43 +1,61 @@ | |||
| var _ = require('lodash'); | |||
| var xmlBuilder = require('xmlbuilder'); | |||
| // @ts-check | |||
There was a problem hiding this comment.
Would it be possible to run a type check for all files with @ts-check on CI somehow? It would be neat.
There was a problem hiding this comment.
I think this can be done with typescript, but at that point may as well convert it all to typescript anyway as I don't think there's limits to the types we can define in JSDOC
According to a best effort spec, it only exists here. See https://github.com/testmoapp/junitxml.
To fix type warning for override with changed signature.
|
@SimeonC, thanks again! I'm now happy with the current state of the PR, so I'll merge and release it now. I'd still appreciate if you took the time to review my comments above, primarily the unresolved ones. |
|
Thanks for cleaning up and merging! (just got back to PC this morning now) |
Adding the JSDoc comments should create support for not needing individual types, at minimum this and the
// @ts-checkcomment gives some extra typechecking without upgrading everything to typescriptFixes #8