Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feature(rome_js_analyze): no function reassign lint rule #2895

Merged
merged 7 commits into from
Jul 19, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Jul 19, 2022

Summary

Closes #2878

Test Plan

> cargo rome-cli check crates/rome_js_analyze/tests/specs/js/noFunctionAssign.js 

@xunilrj xunilrj requested a review from leops as a code owner July 19, 2022 06:33
@xunilrj xunilrj temporarily deployed to aws July 19, 2022 06:33 Inactive
@xunilrj xunilrj requested a review from a team July 19, 2022 06:34
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 19, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 29cdf8f
Status: ✅  Deploy successful!
Preview URL: https://66249fb4.tools-8rn.pages.dev
Branch Preview URL: https://feature-no-function-reassign.tools-8rn.pages.dev

View logs

@xunilrj xunilrj temporarily deployed to aws July 19, 2022 06:34 Inactive
@github-actions
Copy link

github-actions bot commented Jul 19, 2022

Comment on lines 49 to 74
/// function foo() { var foo = bar; }
/// ```
///
/// ```js
/// function foo(foo) { foo = bar; }
/// ```
///
/// ```js
/// function foo() { var foo; foo = bar; }
/// ```
///
/// ```js
/// var foo = () => {}; foo = bar;
/// ```
///
/// ```js
/// var foo = function() {}; foo = bar;
/// ```
///
/// ```js
/// var foo = function() { foo = bar; };
/// ```
///
/// ```js
/// import bar from 'bar';
/// function foo() { var foo = bar; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I recommend to use our formatters formatting for the example scripts (maybe something we can add to the code gen in the future?) because I find it more difficult to read

var foo = function () {}; foo = bar;

vs

var foo = function () {} 
foo = bar;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an idea. In the meantime, I formatted the code manually.

@xunilrj xunilrj temporarily deployed to aws July 19, 2022 07:31 Inactive
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I left some concern about the diagnostics messaging and the correctness of the rule

@xunilrj xunilrj temporarily deployed to aws July 19, 2022 10:43 Inactive
@xunilrj xunilrj force-pushed the feature/no-function-reassign branch from c1abd92 to ad91f39 Compare July 19, 2022 12:32
@xunilrj xunilrj temporarily deployed to aws July 19, 2022 12:32 Inactive
@xunilrj xunilrj temporarily deployed to aws July 19, 2022 16:30 Inactive
@xunilrj xunilrj temporarily deployed to aws July 19, 2022 21:05 Inactive
@xunilrj xunilrj merged commit b97d5e1 into main Jul 19, 2022
@xunilrj xunilrj deleted the feature/no-function-reassign branch July 19, 2022 21:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noFunctionAssign
4 participants