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

POC for #7784 #7786

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

POC for #7784 #7786

wants to merge 5 commits into from

Conversation

Mouvedia
Copy link
Member

@Mouvedia Mouvedia commented Jun 22, 2024

Which issue, if any, is this issue related to?

#7192, #7784

Is there anything in the PR that needs further explanation?

  • new text
  • old range
  • fallback to start/end
  • fallback to warn range (i.e. the new editInfo property is optional)
  • does not require fix to be called (store the data even for the warn path)
  • handles non-consecutive ranges (e.g. declaration-block-no-redundant-longhand-properties)

What's the purpose of this PR?

  1. To bikeshed the name.
    I chose editInfo but I am sure there's something better.
    ref: https://eslint.org/docs/latest/integrate/nodejs-api#-editinfo-type

  2. To be on the same page before I remove the range that are currently returned by the fixers of the rules that have been already refactored.
    i.e. confirm the type of the fix function

  3. to discuss if editInfo should be set on fix or passed directly to report or returned by fix

What is this PR not about?

  • the refactor of alpha-value-notation
  • the test errors of the other rules

Copy link

changeset-bot bot commented Jun 22, 2024

⚠️ No Changeset found

Latest commit: d09fa0a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

range?: Range;
fixed: boolean;
};
type FixerData =
Copy link
Member Author

@Mouvedia Mouvedia Jun 22, 2024

Choose a reason for hiding this comment

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

@ybiquitous @romainmenke
Once you have read OP, can you tell me if this type and Fix correspond to everything we laid out?

Copy link
Member

Choose a reason for hiding this comment

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

Can we spend a bit more time in design phase before creating a POC?

Copy link
Member Author

@Mouvedia Mouvedia Jun 22, 2024

Choose a reason for hiding this comment

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

The POC is there to help the design phase going forward.
We have a concrete example that we can use to iterate further.
Apart from the name and where editInfo should be set/passed, every demand you laid out should be fulfilled.
Can you at least review the index.d.ts and give me your feedback?
This PR is not meant to be merged as is.

Copy link
Member

Choose a reason for hiding this comment

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

I personally do not feel that code review is the most efficient way to design architectural changes like these. This is too taxing for me. Maybe someone else has an easier time to do design work as code review?

Copy link
Member Author

@Mouvedia Mouvedia Jun 22, 2024

Choose a reason for hiding this comment

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

Ill just copy paste the types in the issue then.
For me, at least, it would seem way easier to comprehend with the help of a concrete example.
Ill leave the PR open so that @ybiquitous can give his feedback.

Copy link
Member Author

Choose a reason for hiding this comment

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

alpha.value = fixed;
setDeclarationValue(decl, parsedValue.toString());

/** @type {Problem['fix']} */
Copy link
Member Author

@Mouvedia Mouvedia Jun 24, 2024

Choose a reason for hiding this comment

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

Unless you pass fix directly to report, with the proposal 3, you will have to add the type for every const fix declarations. That's a big CON but not a blocker.

types/stylelint/index.d.ts Outdated Show resolved Hide resolved
Comment on lines +105 to +106
/** @type {Problem['fix']} */
const fix = (apply) => {
Copy link
Member Author

@Mouvedia Mouvedia Jun 27, 2024

Choose a reason for hiding this comment

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

questions
minor
@romainmenke @ybiquitous

I find it annoying that you have to add the type for every single const fix declaration.
Can it be avoided?
Maybe having to set the @param to {boolean} for all fixable rules is not such a big deal?

Are we sure we want the default to be apply?
i.e. it could also be doNotApply

Is this really superior/preferable to returning a function?
e.g.

{
  range?: Range;
  text:  string;
  mutate: () => void | never; 
}

Copy link
Member

Choose a reason for hiding this comment

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

I find it annoying that you have to add the type for every single const fix declaration.
Can it be avoided?

Inlining the fix variable is a way. Otherwise, I think some type annotations are necessary.

For other questions about the function signature, we're discussing it on #7784. Let's continue there.

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

Successfully merging this pull request may close these issues.

None yet

3 participants