Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Conversation

@inancgumus
Copy link
Collaborator

This is getting hairy so a map makes sense to me.

@inancgumus inancgumus added this to the v0.9.0 milestone Feb 16, 2023
@inancgumus inancgumus requested review from ankur22 and ka3de February 16, 2023 09:30
@inancgumus inancgumus self-assigned this Feb 16, 2023
@inancgumus inancgumus changed the base branch from main to fix/mapping-resp-json February 16, 2023 09:31
@inancgumus inancgumus added the mapping k6 browser to Goja mapping related. label Feb 16, 2023
@ka3de
Copy link
Collaborator

ka3de commented Feb 20, 2023

I don't particularly see the need for this change, but don't have strong opinion against it. Do you think it makes it more readable with a map? From the performance perspective, the switch implementation is better, but the impact is negligible, especially being in a test, so happy with any version.

@inancgumus
Copy link
Collaborator Author

@ka3de Yes, I think it's more readable and manageable than a bunch of if statements. It's a small change and I don't have a strong opinion either, can revert it if you guys don't like it, no worries.

@andrewslotin
Copy link
Contributor

While the performance-wise maps and switch statements are pretty much equal, IIRC a method that contains switch only is inlined. Is this the same for map?

@inancgumus
Copy link
Collaborator Author

Hey, @andrewslotin,

While the performance-wise maps and switch statements are pretty much equal, IIRC a method that contains switch only is inlined. Is this the same for map?

Yes, it's the same as long as the function's complexity is low.

Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Base automatically changed from fix/mapping-resp-json to main February 21, 2023 07:26
This is getting hairy so a map makes sense.
@inancgumus inancgumus force-pushed the refactor/mapping-toFirstLetterLower branch from eab18d3 to 0ae492d Compare February 21, 2023 12:26
@inancgumus inancgumus merged commit 26d0b83 into main Feb 21, 2023
@inancgumus inancgumus deleted the refactor/mapping-toFirstLetterLower branch February 21, 2023 12:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

mapping k6 browser to Goja mapping related. refactor tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants