Skip to content

fix: issue #6813 (Zero-based lists are broken)#6852

Merged
alexander-akait merged 2 commits intomasterfrom
issue-6813
Nov 14, 2019
Merged

fix: issue #6813 (Zero-based lists are broken)#6852
alexander-akait merged 2 commits intomasterfrom
issue-6813

Conversation

@alexander-akait
Copy link
Copy Markdown
Member

@alexander-akait alexander-akait commented Nov 6, 2019 •
edited
Loading

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

✨Try the playground for this PR✨

fixes #6813 + many tests

@alexander-akait alexander-akait force-pushed the issue-6813 branch 2 times, most recently from dfe9e6f to 2a23b71 Compare November 6, 2019 15:30
@alexander-akait
Copy link
Copy Markdown
Member Author

alexander-akait commented Nov 7, 2019

/cc @prettier/core somebody can review 😄 ?

j-f1
j-f1 requested changes Nov 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

A couple suggested renames & a comment about the algorithm:

@alexander-akait
Copy link
Copy Markdown
Member Author

alexander-akait commented Nov 8, 2019 •
edited
Loading

/cc @j-f1 can you approve?

@fisker
Copy link
Copy Markdown
Member

fisker commented Nov 8, 2019

@evilebottnawi

how about

const [firstNumber, secondNumber, thirdNumber] = node.children
  .slice(0, 3)
  .filter(Boolean)
  .map(child =>
    Number(getOrderedListItemInfo(child, options.originalText).numberText)
  );

@alexander-akait
Copy link
Copy Markdown
Member Author

alexander-akait commented Nov 8, 2019

thirdNumber may not exist, also it is unnecessary get third node when list contains only 2 items

@fisker
Copy link
Copy Markdown
Member

fisker commented Nov 8, 2019

I used .filter(Boolean)

@lydell lydell added this to the 2.0 milestone Nov 9, 2019
@alexander-akait
Copy link
Copy Markdown
Member Author

alexander-akait commented Nov 9, 2019

@lydell why 2?

@alexander-akait
Copy link
Copy Markdown
Member Author

alexander-akait commented Nov 9, 2019

There are a lot of changes for the second version, we should still have an intermediate version 1.20

@lydell
Copy link
Copy Markdown
Member

lydell commented Nov 9, 2019 •
edited
Loading

The plan is to release 2.0 early January 2020. I don’t think we’ll be able to jam another release in between. It’s just two months away and there’s Christmas and all.

There are a lot of changes for the second version

Which changes are you referring to? Have you seen the updated plans?

@alexander-akait
Copy link
Copy Markdown
Member Author

alexander-akait commented Nov 13, 2019

/cc @lydell should i use master because it is not breaking change?

@lydell
Copy link
Copy Markdown
Member

lydell commented Nov 13, 2019

Yes, sounds good.

alexander-akait
alexander-akait commented Nov 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member Author

@alexander-akait alexander-akait left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/cc @lydell ready for review
/cc @j-f1

@thorn0 thorn0 changed the title fix: issue #6813 fix: issue #6813 (Zero-based lists are broken) Nov 14, 2019
fisker
fisker reviewed Nov 14, 2019
View reviewed changes
src/language-markdown/utils.js
return { numberText, marker, leadingSpaces };
}

function hasGitDiffFriendlyOrderedList(node, options) {
Copy link
Copy Markdown
Member

@fisker fisker Nov 14, 2019

Choose a reason for hiding this comment

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

why not name hasDiffFriendlyOrderedList

Copy link
Copy Markdown
Member Author

@alexander-akait alexander-akait Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Because it is git diff, not diff, it is original name and i keep it, so it is out of this PR

lydell
lydell reviewed Nov 14, 2019
View reviewed changes
@alexander-akait alexander-akait merged commit 61a2482 into master Nov 14, 2019
@alexander-akait alexander-akait deleted the issue-6813 branch November 14, 2019 17:59
lipis added a commit that referenced this pull request Jan 8, 2020
@lipis
Merge branch 'next' of github.com:prettier/prettier into next
ac6c6fa 
* 'next' of github.com:prettier/prettier:
  Optimize some usage of `Array#filter` (#6996)
  Update `jest` to v24 (#6954)
  Replace `trim{Left,Right}` with `trim{Start,End}` (#6994)
  Set `trailingComma` default value to `es5` (#6963)
  Fix `new` usage for builtin objects (#6968)
  Replace `indexOf` with `includes` (#6967)
  fix: tests for empty type parameters in TS (#6960)
  Fix MDX html parsing errors (#6949)
  fix: issue #6813 (Zero-based lists are broken) (#6852)
  Style: use async functions (#6935)
  Disable trailingComma for Angular internal parser (#6912)
  Update `snapshot-diff` to v0.6.1 (#6955)
  Update build scripts to target Node.js 10 (#6908)
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 12, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@fisker fisker fisker left review comments

@lydell lydell lydell approved these changes

@j-f1 j-f1 j-f1 approved these changes

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

[Markdown] Zero-based lists are broken

4 participants

@alexander-akait @fisker @lydell @j-f1