fix: resolved 15 ESLint warnings #2065
Conversation
sdthaker
force-pushed
the
Fix-lint-warnings-in-blockly-samples-#1978
branch
from
9b9369b to
0f7ff79
Compare
sdthaker
changed the title
maribethb
left a comment
maribethb
left a comment
There was a problem hiding this comment.
Thanks for this PR! Unfortunately getting the types correct is a bit trickier than it looks, but I've left some pointers below. Let me know if you have questions!
plugins/migration/bin/fix-imports.js
Outdated
|
|
||
| // TODO (#1211): Make this database format more robust. | ||
| /** @type {MigrationData[]} */ | ||
| /** @type {Array.<MigrationData>} */ |
There was a problem hiding this comment.
We don't use the . after Array so this should be Array<MigrationData>.
| loadExtraState: function ( | ||
| this: DynamicListCreateBlock, | ||
| state: {[x: string]: any} | string, | ||
| state: {[x: string]: number} | string, |
There was a problem hiding this comment.
I don't think this is quite right for the state types, here and below. The extraState could have any number of properties on it (we don't know) and it's not correct to assume all of those properties will be numbers (or strings, or whatever). We don't know the types. To be strict, we should type that as unknown rather than any.
I think the most correct would be
state: { itemCount?: number, [x: string]: unknown} | string,
This says the state can either be a string (when it's xml), or it can be an object with any number of properties whose type is unknown, and also it might have a property called itemCount that's a number. Then you'll get an error later because unknown is a stricter type than any. You'll need to change this.itemCount = state['itemCount']; to this.itemCount = state['itemCount'] ?? 0; to properly account for the case where itemCount does not exist in the extraState.
This might seem like a lot of work, but now the code will be truly type-safe instead of the current state which uses any to basically turn off type checking.
You would need a similar fix for the others below, but instead of itemCount it would be whatever property is actually used in the body of the function.
| createPlayground( | ||
| document.getElementById('root'), | ||
| createWorkspace as ( | ||
| blocklyDiv: HTMLElement, |
There was a problem hiding this comment.
I don't think you need this cast - createWorkspace already matches that type as far as I can tell. I think the original cast as any was maybe just unnecessary.
scripts/gh-predeploy.js
Outdated
| if (!demoConfig) { | ||
| done(); | ||
| return; | ||
| return done(); |
There was a problem hiding this comment.
I don't think this is correct; we don't want to return the value of this function (I don't even know what that value would be, it's just arcane gulp magic).
If we're returning early then we actually shouldn't return a gulp task. The types here are super unimportant. I think you can change the return type to be {*} or maybe {Function | undefined}
* added explicit type as number to state parameter in loadExtraState() * added explicit type as number to state parameter in loadExtraState() * positioned variable declaration in jsdoc to its correct place * added jsdoc comment for getScaledBBox() function * added explicit false return statement instead of returning undefined * jsdoc: updated type declaraion, added missing returns type, added missing identifier and its type * slienced the warning based on jsdoc comment for object's class declaration * slienced the warning based on jsdoc comment for object's class declaration * slienced the warning based on jsdoc comment for object's class declaration * removed unused import, updated function declaration in a function's arguments * removed empty return statment, now returning result of done function call
* removed dot after Array declaration * removed the explicit cast when passing createWorkspace as an argument * restored done() invocation, updated return statement in jsdoc to return Function or undefined * updated state type to include itemCount:number prop, added nullish coalescing for proper assignment * updated state type to include itemCount:number prop, added nullish coalescing for proper assignment
sdthaker
force-pushed
the
Fix-lint-warnings-in-blockly-samples-#1978
branch
from
0f7ff79 to
e2e3073
Compare
@maribethb Wow! This is amazing feedback! I've never received detailed feedback of this sort, considering I'm quite early in this career and new to open source! Thanks so much for taking the time to explain in detail what should be the code changes and the impact of those changes. I've made changes and pushed the updated code! Please take a look and let me know if anything else needs to be added/removed from this PR. |
maribethb
left a comment
maribethb
left a comment
There was a problem hiding this comment.
Awesome, this looks great. Thanks for your work on this!
2 participants
The basics
The details
Resolves
Fixes #1978
Proposed Changes
Fixes these warnings (some of them were repeated warnings):
Reason for Changes
These changes are done because aligning the code with the linting rules allows for the static analysis tool to catch any uncaught bugs at compile time.
Test Coverage
npm run testresults:npm run buildresults:Documentation
Additional Information
The only warning left to be fixed is,