Start using eslint-plugin-deprecation to find deprecated code #3275
Conversation
|
|
||
| const manyScannedRepos = Array.from({ length: 1000 }, (_, i) => { | ||
| const mockedScannedRepo = createMockScannedRepo(); | ||
| const randomInt = uniqueNumbers.pop(); | ||
| const uniqueId = | ||
| randomInt === undefined ? Math.random() * 8000 + 1001 : randomInt; |
There was a problem hiding this comment.
Since array.pop() returns a number OR undefined I needed to add an alternative for the undefined case. This is a best effort for a random number bigger than 1000 without using a library to create it. This case should never happen since the uniqueNumbers array is never empty.
There was a problem hiding this comment.
I'm not a huge fan of having code that's not intended to execute, and that branch isn't just throwing an error. I have a few potential solutions to make things a little cleaner:
Throw an error instead
I'd be happy if we threw an error if pop ever returns undefined. As you say it shouldn't ever happen, so if it does then the code needs to be updated rather than using a fallback strategy.
Iterate over uniqueNumbers
Instead of doing Array.from({ length: 1000 }, (_, i) => to initiate the loop, do uniqueNumbers.map(repoId => so that we remove the need to call pop.
Just use Math.random() and don't precompute a random list
The repo ids don't have to go from 0-999. They just need to be unique, and even then it's only the storybook so not too critical. If we use big enough numbers, e.g. Math.floor(Math.random() * 1000000000000) (AFAIK the max safe int in javascript is 9007199254740991), then the chance of getting duplicates in our 1000 repo ids becomes very low.
There was a problem hiding this comment.
I think I prefer the error, as you say it should never happen and if it does we should look at it!
extensions/ql-vscode/src/stories/variant-analysis/VariantAnalysisAnalyzedRepos.stories.tsx
Outdated
Show resolved
Hide resolved
| id: uniqueId, | ||
| fullName: `octodemo/${uniqueId}`, | ||
| // We need to ensure the ID is unique for React keys | ||
| id: parseInt(nanoid()), |
There was a problem hiding this comment.
Is parseInt a left over from before? nanoid() gives you back a string, not a number.
There was a problem hiding this comment.
Yes, that's why I have to convert it to a number for the id. Nanoid has no method that returns a number, however by giving it the custom alphabet on line 132 we can parse the string back to a number.
There was a problem hiding this comment.
Ok I'm now following a bit more what we're trying to do 😂 I didn't realise this is an id for a repo (which is expected to be a number). Although I'm still unclear why we aren't just using Math.random() 🤔
I think what you have is fine and I think it'll work because there are enough characters to avoid collisions. This is also storybook code so it's not terrible if things break 😬
Do we want to skip 0 since you could end up with a number of 0123? parseInt still works but it doesn't skip the 0, it just assumes the number is octal rather than decimal so can be a bit confusing.
Adds the plugin and resolves existing issues. In two cases the deprecated methods couldn't be easily replaced.
Checklist
ready-for-doc-reviewlabel there.