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

feat: custom rule no redundant commit #25

Merged
merged 1 commit into from
Dec 28, 2023
Merged

Conversation

SeanSilke
Copy link
Contributor

@SeanSilke SeanSilke commented Dec 27, 2023

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @salutejs/[email protected]
npm install @salutejs/[email protected]
npm install @salutejs/[email protected]
# or 
yarn add @salutejs/[email protected]
yarn add @salutejs/[email protected]
yarn add @salutejs/[email protected]

@SeanSilke SeanSilke requested a review from KateKate December 27, 2023 07:59
},
"publishConfig": {
"access": "public"
},
"devDependencies": {
"eslint": ">=8.54.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

eslint не должно быть тут, оно уже в peerDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Он нужен что-бы локально прогонять тесты на правила.

* `IsAnonymousFunctionDefinition()` in the ECMAScript spec you'll find places
* where JS gives anonymous function expressions names. We roughly detect the
* same AST nodes with some exceptions to better fit our use case.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some improvement from copilot

/**
 * Retrieves the static name of a function AST node. Straightforward for function declarations, 
 * but complex for anonymous function expressions. It emulates the `IsAnonymousFunctionDefinition()` 
 * from the ECMAScript spec, with modifications for our use case.
 */

Copy link
Contributor Author

@SeanSilke SeanSilke Dec 27, 2023

Choose a reason for hiding this comment

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

This code i copy paste from react repo.
See link before declaration of function.


// NOTE: We could also support `ClassProperty` and `MethodDefinition`
// here to be pedantic. However, hooks in a class are an anti-pattern. So
// we don't allow it to error early.
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут не поняла, hooks вообще не могут быть использованы в классах. это не anti-pattern, оно просто не работает

Copy link
Contributor Author

@SeanSilke SeanSilke Dec 27, 2023

Choose a reason for hiding this comment

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

This code i copy paste from react repo.
See link before declaration of function.

/**
* Checks if the node is a React component name. React component names must
* always start with an uppercase letter.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Checks if the node is a valid React component name, which must start with an uppercase letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also original text. I don't let link to original because code is pretty straightforward.

meta: {
type: null, // `problem`, `suggestion`, or `layout`
docs: {
description: "asdf",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"asdf" - ?

docs: {
description: "asdf",
recommended: false,
url: null, // URL to the documentation page for this rule
Copy link
Collaborator

Choose a reason for hiding this comment

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

) {
context.report({
node,
message: "Don't use synchronous state settlers in effects",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
message: "Don't use synchronous state settlers in effects",
message: "Avoid using synchronous state setters within effects",

});
}

// checking is it is useEffect call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// checking is it is useEffect call
// Checking if the invoked function is useEffect

isInUseEffect = true;
}

// checking is it is useState call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// checking is it is useState call
// Checking if the function call is useState

isInUseEffect = false;
}

// Current implementation don't take into account nested setTimeout
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Current implementation don't take into account nested setTimeout
// The current implementation does not consider nested setTimeout calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Точка в конце коммента. Мое уважение!

const name = getFunctionName(node);

if (name && isComponentName(name)) {
// We doesn't take into account nested component declaration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// We doesn't take into account nested component declaration
// We doesn't consider nested component declaration

const name = getFunctionName(node);

if (name && isComponentName(name)) {
// We doesn't take into account nested component declaration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// We doesn't take into account nested component declaration
// We doesn't consider nested component declaration

@@ -0,0 +1,19 @@
/* config/eslint/index.js */
Copy link
Collaborator

Choose a reason for hiding this comment

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

зачем это?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Уберу.

@SeanSilke SeanSilke merged commit 5b51a31 into master Dec 28, 2023
5 checks passed
@SeanSilke SeanSilke deleted the no-redundant-commit branch December 28, 2023 10:50
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.

2 participants