Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(js_formatter): updated the is_test_call_expression function to match prettier #4811

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mdm317
Copy link
Contributor

@mdm317 mdm317 commented Dec 30, 2024

Summary

fixes #4202

https://github.com/prettier/prettier/blob/d9d0999aac70c04b06b35cbcb2c804b4a9c8d8a8/src/language-js/utils/index.js#L383-L387

For Prettier, assume it is a Testcall only if the first argument is a StringLiteral or TemplateLiteral.
So, I added the logic.
However, assuming that it is not a Testcall if the first argument is neither a StringLiteral nor a TemplateLiteral might have some edge cases.
Please share your thoughts.

Test Plan

add test

@mdm317 mdm317 marked this pull request as draft December 30, 2024 10:20
@github-actions github-actions bot added A-Parser Area: parser A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels Dec 30, 2024
@github-actions github-actions bot removed the A-Parser Area: parser label Jan 2, 2025
Copy link

codspeed-hq bot commented Jan 2, 2025

CodSpeed Performance Report

Merging #4811 will not alter performance

Comparing mdm317:fix/4202 (beba389) with main (a658a29)

Summary

✅ 95 untouched benchmarks

@mdm317
Copy link
Contributor Author

mdm317 commented Jan 2, 2025

in ci

error: test failed, to rerun pass -p biome_js_formatter --test prettier_tests

I don't understand why this error occurs, even though the issue-4202.js.prettier-snap file exists.
Can someone help me?

@mdm317 mdm317 marked this pull request as ready for review January 2, 2025 16:27
Copy link
Contributor

@dyc3 dyc3 Jan 2, 2025

Choose a reason for hiding this comment

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

The tests in specs/prettier are synced with the prettier repo. You should move your new snapshot tests outside of that folder into specs/js.

Copy link
Member

Choose a reason for hiding this comment

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

@mdm317 Also, you should add template literals to the tests, since they are part of the new logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dyc3
Thank you for letting me know. It was really helpful!

@ematipico
Thank you for the review. I have one question.

it(`does something really long and complicated so I have to write a very long name for the test`, function () {
console.log("hello!");
});

Template literals are already exist, and the results won't change.
Should I still add them?
If I misunderstood something, please let me know.

@mdm317 mdm317 requested review from dyc3 and ematipico January 5, 2025 14:36
@ematipico
Copy link
Member

@mdm317 can you please add another test case with template literals?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📝 Body of the function is formatted with the same indent as previous line
3 participants