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

Handle functions and assignment in prefer-at condition #114

Merged
merged 6 commits into from
Aug 23, 2024

Conversation

ShridharGoel
Copy link
Contributor

@ShridharGoel ShridharGoel commented Aug 15, 2024

Expensify/App#43055

This is a follow-up to include handling of functions like map and assignment like a[i] = 2.

Comment on lines 80 to 88
// Skip if the property is a method (like a.map)
if (node.parent && node.parent.type === AST_NODE_TYPES.CallExpression && node.parent.callee === node) {
return;
}

// Skip if the node is part of an assignment expression (like a[i] = 2)
if (isAssignmentExpression(node)) {
return;
}
Copy link

Choose a reason for hiding this comment

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

We want to return early if node is left-hand side right? If so, I think it'll be better to wrap them in method. I'm not sure if there's a similar method from @typescript-eslint/utils

Copy link

Choose a reason for hiding this comment

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

@ShridharGoel I did some search and found a similar implementation from this project eslint-plugin-unicorn (under MIT license) and there's a isLeftHandSide method. Maybe we can learn some ideas from there to make our implementation more robust.

@fabioh8010
Copy link
Contributor

@ShridharGoel Could you please take a look at @eh2077 comments?

@ShridharGoel
Copy link
Contributor Author

Updated.

@fabioh8010
Copy link
Contributor

@eh2077 Could you take a look now? Thanks 😄

@eh2077
Copy link

eh2077 commented Aug 23, 2024

@ShridharGoel The unit test is failing. Can you please take a look? Seems not related to this change?

Screenshot image

@fabioh8010
Copy link
Contributor

@eh2077 This unit test is already failing on main IIRC, I think we can ignore it.


function isLeftHandSide(node) {
return (
(node.parent.type === 'AssignmentExpression' || node.parent.type === 'AssignmentPattern')
Copy link

Choose a reason for hiding this comment

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

NAB: We use constants from

const { AST_NODE_TYPES, ESLintUtils } = require('@typescript-eslint/utils');

in prefer-at.js.

Copy link

@eh2077 eh2077 left a comment

Choose a reason for hiding this comment

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

LGTM!

I ran test and lint locally but both of them failed though they're not caused by this change.

@fabioh8010 Can we just ignore them in this PR?

@fabioh8010
Copy link
Contributor

Yes I would ignore them for now

@roryabraham roryabraham merged commit fc5dc90 into Expensify:main Aug 23, 2024
Copy link
Contributor

🚀 Published in 2.0.59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants