zip file viewer: omit profile extension if it is .json#5079
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5079 +/- ##
=======================================
Coverage 88.49% 88.49%
=======================================
Files 304 304
Lines 27461 27467 +6
Branches 7430 7431 +1
=======================================
+ Hits 24302 24308 +6
Misses 2934 2934
Partials 225 225 ☔ View full report in Codecov by Sentry. |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks for the patch and thanks for your patience, sorry for the delay in reviewing the patch!
Here are some comments, please tell me what you think!
| ].forEach((fileName) => screen.getByText(fileName)); | ||
| }); | ||
|
|
||
| it('removes .json and .json.gz extensions', async () => { |
| await setup(); | ||
|
|
||
| ['profile1', 'profile2', 'profile3', 'profile4'].forEach((fileName) => | ||
| screen.getByText(fileName) |
There was a problem hiding this comment.
We use the jest-dom library, which makes it possible to be somewhat more explicit in our expectations, with something such as:
expect(screen.getByText(..)).toBeInTheDocument()
Normally there's a eslint rule for that, but I think it doesn't find it because of the forEach
There was a problem hiding this comment.
I was aiming to match the style in the existing test above, but being more explicit is my preference too. I've gone ahead and updated both tests.
There may be a reason it was like this that I am overlooking though, so please let me know if you'd prefer it to be changed back!
There was a problem hiding this comment.
ah yes I see
I believe the main reason is that this wasn't caught by the eslint rule :-)
Thanks for updating
| 'profile3.json.gz', | ||
| 'profile4.json', | ||
| ].forEach((fileName) => { | ||
| expect(() => screen.getByText(fileName)).toThrow(); |
There was a problem hiding this comment.
Similarly it would be more explicit to write it like this:
expect(screen.queryByText(fileName)).not.toBeInTheDocument()
| it('preserves extensions other than .json and .json.gz', async () => { | ||
| await setup(); | ||
|
|
||
| expect(screen.getByText('profile.pdf')).toBeInTheDocument(); |
| const EXTENSIONS_TO_OMIT = ['.json', '.json.gz']; | ||
| const matchedExtension = EXTENSIONS_TO_OMIT.find((extension) => | ||
| name.endsWith(extension) | ||
| ); | ||
| if (matchedExtension) { | ||
| name = name.slice(0, name.length - matchedExtension.length); | ||
| } |
There was a problem hiding this comment.
I would have done it with a regexp such as:
let name = ...
const extensionsToOmitRe = /\.json(\.gz)?$/;
name = name.replace(extensionsToOmitRe, "");It's terser but also maybe less explicit for some people.
Your solution is valid too, and I don't think one is better than the other in most cases, so I'll let you decide which one you prefer (just in case you didn't think of the RegExp based solution).
There was a problem hiding this comment.
Thanks for the suggestion!
I tend to lean away from using regex for most cases, so if possible I'd prefer to keep it as is
| } | ||
|
|
||
| let name = this._zipFileTable.partName[zipTableIndex]; | ||
| const EXTENSIONS_TO_OMIT = ['.json', '.json.gz']; |
There was a problem hiding this comment.
oh actually it looks like we don't support .json.gz files inside a zip file, it just doesn't work... So it makes no sense to remove it . Can you change it and just keep ".json"? I think it makes sense to keep an array in case we want to add some more later. (Or otherwise use the Regexp like mentioned in the other comment)
And maybe eventually we'll need to support .json.gz inside a zip...
joshuaobrien
changed the title
[Florian Quèze] differential timestamp compression when serializing processed profiles (#5033) [joshuaobrien] Zip file viewer: omit profile extension if it is .json (#5079) [Paul Adenot] Allow typing '?' in `contenteditable` elements without having the help menu pop in (#5124) [Julien Wajsberg] Enable the Friulian locale (#5127) Thanks also to our localizers! fur: Fabio Tomat nl: simonmeulenbeek tr: Grk, Nazım Can Altınova
2 participants
Closes #4289