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

Merge additional changes from adalinesimonian/merge-prettier-2.3.1 #603

Conversation

adalinesimonian
Copy link
Contributor

@adalinesimonian adalinesimonian commented Jun 16, 2021

Closes #587
Closes #548
Closes #422
Closes #414

Per #569 (comment)

thorn0 and others added 30 commits April 21, 2021 02:45
* Update release dependencies

* Remove `version` field

* Run install before any require

* Update scripts/release/utils.js

Co-authored-by: Sosuke Suzuki <[email protected]>

* Fix update schedule

* Update deps

* Fix

Co-authored-by: Sosuke Suzuki <[email protected]>
…0753)

Bumps [eslint-plugin-unicorn](https://github.com/sindresorhus/eslint-plugin-unicorn) from 29.0.0 to 30.0.0.
- [Release notes](https://github.com/sindresorhus/eslint-plugin-unicorn/releases)
- [Commits](sindresorhus/eslint-plugin-unicorn@v29.0.0...v30.0.0)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [webpack](https://github.com/webpack/webpack) from 5.33.2 to 5.35.0.
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v5.33.2...v5.35.0)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [chalk](https://github.com/chalk/chalk) from 4.1.0 to 4.1.1.
- [Release notes](https://github.com/chalk/chalk/releases)
- [Commits](chalk/chalk@v4.1.0...v4.1.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [mem](https://github.com/sindresorhus/mem) from 8.1.0 to 8.1.1.
- [Release notes](https://github.com/sindresorhus/mem/releases)
- [Commits](sindresorhus/memoize@v8.1.0...v8.1.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [chalk](https://github.com/chalk/chalk) from 4.1.0 to 4.1.1.
- [Release notes](https://github.com/chalk/chalk/releases)
- [Commits](chalk/chalk@v4.1.0...v4.1.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…745)

Bumps [semver](https://github.com/npm/node-semver) from 7.3.4 to 7.3.5.
- [Release notes](https://github.com/npm/node-semver/releases)
- [Changelog](https://github.com/npm/node-semver/blob/master/CHANGELOG.md)
- [Commits](npm/node-semver@v7.3.4...v7.3.5)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [webpack](https://github.com/webpack/webpack) from 5.35.0 to 5.35.1.
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v5.35.0...v5.35.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@glimmer/syntax](https://github.com/glimmerjs/glimmer-vm) from 0.78.0 to 0.78.2.
- [Release notes](https://github.com/glimmerjs/glimmer-vm/releases)
- [Changelog](https://github.com/glimmerjs/glimmer-vm/blob/master/CHANGELOG.md)
- [Commits](glimmerjs/glimmer-vm@v0.78.0...v0.78.2)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: fisker Cheung <[email protected]>
Bumps [@angular/compiler](https://github.com/angular/angular/tree/HEAD/packages/compiler) from 11.2.10 to 11.2.11.
- [Release notes](https://github.com/angular/angular/releases)
- [Changelog](https://github.com/angular/angular/blob/master/CHANGELOG.md)
- [Commits](https://github.com/angular/angular/commits/11.2.11/packages/compiler)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: fisker Cheung <[email protected]>
* Build(deps): Bump linguist-languages from 7.13.0 to 7.14.0

Bumps [linguist-languages](https://github.com/ikatyang/linguist-languages) from 7.13.0 to 7.14.0.
- [Release notes](https://github.com/ikatyang/linguist-languages/releases)
- [Commits](ikatyang/linguist-languages@v7.13.0...v7.14.0)

Signed-off-by: dependabot[bot] <[email protected]>

* Update snapshot

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: fisker Cheung <[email protected]>
* docs: update git hooks instructions in install.md

* Add Yarn 2 link
* Exclude @Keyframes params from being parsed as a Less variable

* Update changelog_unreleased/css/10773.md

Co-authored-by: fisker Cheung <[email protected]>

* Update changelog_unreleased/css/10773.md

Co-authored-by: fisker Cheung <[email protected]>

* Add "keyframes" to cspell.json

Co-authored-by: fisker Cheung <[email protected]>
@adalinesimonian
Copy link
Contributor Author

I have some concerns about priorities here, but as I will be unavailable from now until the 5th of July, I will voice my concerns if they are still relevant at that time at the earliest. I will be out on the streets working every day throughout the record-breaking heatwave we are experiencing here in Seattle to get critical supplies to people and get people to safety where possible. I will be taking a week to recover afterwards.

@brodycj
Copy link
Owner

brodycj commented Jun 28, 2021

Thanks. Yes I have been taking this work through a few too many circles and will try to bring some more forward progress.

I have been working pretty hard to keep things as clean and well tested as possible, unfortunately have sacrificed too much forward progress in the process.

brodycj added a commit that referenced this pull request Jun 28, 2021
into a better-organized tests/format/x subdirectory tree

move some TypeScript-specific test subdirectories under
tests/format/x/typescript

rename some test subdirectories

should help avoid lint issues with updates from upstream Prettier

based on some updates proposed in:

- #603

NEXT STEP: move prettierX-specific test variations into tests/format/x
subdirectory tree

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 28, 2021
into a better-organized tests/format/x subdirectory tree

move some TypeScript-specific test subdirectories under
tests/format/x/typescript

rename some test subdirectories

should help avoid lint issues with updates from upstream Prettier

based on some updates proposed in:

- #603

NEXT STEP: move prettierX-specific test variations into tests/format/x
subdirectory tree

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 28, 2021
into tests/format/x subdirectory tree

based on some updates proposed in:

- #603

update test comments

rename dirpath -> dirPath to avoid spellcheck issues with
updates from upstream Prettier

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 28, 2021
(in prettierx-rebase-branch-001 version branch)

as proposed in:

- #569
- #603

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 28, 2021
(in prettierx-rebase-branch-001 version branch)

using file link for internal eslint plugin in package.json

(link: paths are only supported by Yarn)

based on a proposal in:

- #603

and update tests/integration/env.js to use fork package name
from package.json

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 28, 2021
(in prettierx-rebase-branch-001 version branch)

support pnpm test with some script improvements

(dev package test with pnpm)

refactor scripts/install-prettier.js with optional dep support

based on changes proposed in:

- #603

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
"? ",
// [prettierx] spaceInParens option support (...)
...(consequentNode.type === node.type
? [ifBreak("", "("), parenSpace]
Copy link
Owner

@brodycj brodycj Jun 28, 2021

Choose a reason for hiding this comment

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

minor fix is needed here for consistency with existing prettierX version 0.18.x:

Suggested change
? [ifBreak("", "("), parenSpace]
? [ifBreak("", ["(", parenSpace])]

I will make this fix.

brodycj added a commit that referenced this pull request Jun 28, 2021
into prettierx-rebase-branch-001

based on some updates from:

- #549
- #569
- #603

(with very limited update needed in src/language-js/needs-parens.js)

**tested** with prettierX-specific test cases from prettierX 0.18.x

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
into prettierx-rebase-branch-001

based on some updates from:

- #549
- #569
- #603

(with a very limited update needed in src/language-js/needs-parens.js)

**tested** with prettierX-specific test cases from prettierX 0.18.x

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
to be more consistent with prettierX 0.18.x

including some updates from:

- #549
- #569
- #603

Co-authored-by: Christopher J. Brody <[email protected]>
Co-authored-by: Adaline Valentina Simonian <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
to be more consistent with prettierX 0.18.x

including some updates from:

- #549
- #569
- #603

Co-authored-by: Christopher J. Brody <[email protected]>
Co-authored-by: Adaline Valentina Simonian <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
into prettierx-rebase-branch-001

based on some updates from:

- #549
- #569
- #603

(with a very limited update needed in src/language-js/needs-parens.js)

**tested** with prettierX-specific formatting test cases from
prettierX 0.18.x

(discovered a very limited number of changes from prettierX 0.18.x)

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
into prettierx-rebase-branch-001

based on some updates from:

- #549
- #569
- #603

(with a very limited update needed in src/language-js/needs-parens.js)

**tested** with prettierX-specific formatting test cases from
prettierX 0.18.x

(discovered a very limited number of changes from prettierX 0.18.x)

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
to be more consistent with prettierX 0.18.x

including some updates from:

- #549
- #569
- #603

Co-authored-by: Christopher J. Brody <[email protected]>
Co-authored-by: Adaline Valentina Simonian <[email protected]>
brodycj added a commit that referenced this pull request Jun 29, 2021
into prettierx-rebase-branch-001

based on some updates from:

- #549
- #569
- #603

with a very limited update needed in src/language-js/needs-parens.js

and with comments where the updates are NO LONGER NEEDED in
src/language-js/needs-parens.js

tested with prettierX-specific formatting test cases from prettierX
0.18.x (discovered a very limited number of deviations)

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 30, 2021
into prettierx-rebase-branch-001

based on some updates from:

- #549
- #569
- #603

with a very limited update needed in src/language-js/needs-parens.js

and with comments where the updates are NO LONGER NEEDED in
src/language-js/needs-parens.js

tested with prettierX-specific formatting test cases from prettierX
0.18.x (discovered a very limited number of deviations)

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jun 30, 2021
into prettierx 0.19.0-01-update-branch

based on some updates from:

- #549
- #569
- #603

with a very limited update needed in src/language-js/needs-parens.js

and with comments where updates from 0.18.x are NO LONGER NEEDED in
src/language-js/needs-parens.js

tested with prettierX-specific formatting test cases from prettierX
0.18.x (discovered a very limited number of deviations)

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
brodycj added a commit that referenced this pull request Jul 1, 2021
into prettierx 0.19.0-01-update-branch

based on some updates from:

- #549
- #569
- #603

with a very limited update needed in src/language-js/needs-parens.js

and with comments where updates from 0.18.x are NO LONGER NEEDED in
src/language-js/needs-parens.js

tested with prettierX-specific formatting test cases from prettierX
0.18.x (discovered a very limited number of deviations)

TODO items:

- include test updates from prettierX dev branch
- update documentation

Co-authored-by: Adaline Valentina Simonian <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
@brodycj
Copy link
Owner

brodycj commented Jul 2, 2021

Hello @adalinesimonian please be sure to stay safe in this hot weather, especially with the upcoming holiday!

I have been working pretty hard to rebase and clean these updates up in PR #630. I will likely merge these updates sometime tomorrow and then let the Renovate bot update as many dependencies as possible before making the release.

I am at the point where I would like to make the release over the holiday and move on. I know you have worked quite hard on this, and it has really distracted me from both free and professional software commitments over the past month.

There were a few things that I did, mostly in the CI testing, differently than how it was proposed here. But if you think I missed any important updates please feel free to raise one or more new PRs for disucssion.

And thanks again for your contribution, which I am sure will benefit many users.

brodycj added a commit that referenced this pull request Jul 6, 2021
for contributions in PR #603 that enabled update from Prettier 2.3.2
@brodycj
Copy link
Owner

brodycj commented Jul 6, 2021

@adalinesimonian I hope you are safe after the heat and the long weekend!

I have incorporated most of the proposed changes in PR #630 and gave you some credit in README.md. I did solve some CI issues a different way, unfortunately might have missed some of your changes in the process. I have raised issue #636 to start tracking some followup items needed. Please DO feel free to raise another PR if you think I missed anything important.

These updates have taken me almost a whole month to incorporate due to the work that I put into breaking some changes down to smaller updates, with help of some intermediate "version branches". I have ended up sacrificing some badly needed maintenance on some other open-source projects and have also delayed responding to some people interested in helping me find some part-time work as well.

But I would like to give thanks to you @adalinesimonian for the work on your part, which has enabled this update in the first place, which I think will help many people in the near future.

@adalinesimonian
Copy link
Contributor Author

@adalinesimonian I hope you are safe after the heat and the long weekend!

I am; thank you for your concern. Luckily, in my effort to distribute water and supplies, I only encountered minor heat exhaustion symptoms on several days, after which I immediately retreated home to recover. The fireworks I got to see this weekend made it all worth it.

I have incorporated most of the proposed changes in PR #630 and gave you some credit in README.md.

You undoubtedly did a lot of work! I appreciate the credit; it was certainly not necessary but thank you, nonetheless!

I did solve some CI issues a different way, unfortunately might have missed some of your changes in the process. I have raised issue #636 to start tracking some followup items needed. Please DO feel free to raise another PR if you think I missed anything important.

These updates have taken me almost a whole month to incorporate due to the work that I put into breaking some changes down to smaller updates, with help of some intermediate "version branches". I have ended up sacrificing some badly needed maintenance on some other open-source projects and have also delayed responding to some people interested in helping me find some part-time work as well.

I acknowledge that you've made more granular commits, which may, in certain circumstances, make it easier to identify specific sets of changes by making it such that the history refers to smaller groups of changes. However, after looking more closely at this effort, I don't believe this effort was worth the massive delay and other issues that it added to integrating changes from upstream. I find this especially true given that this will be far from the last time that there are significant changes upstream that we need to integrate down here.

We should have worked together in a unified set of changes throughout this effort so that our efforts were not doubled or split and kept shared knowledge together in one or a few places. Instead, that work became fragmented and introduced many points where there was a chance to miss things. This fragmentation also adds to the workload and delay (addressed in the following points).

  • Multiplied points of failure — to be clear, what I mean as a point of failure in this context is a chance to integrate changes with mistakes or miss changes that are important. Each time you copied work into separate branches over and over again, it introduced the possibility of missing changes or implementing changes in ways that missed the nuance of or reasons for changes that were re-implemented. This possibility, ultimately, was not just a possibility, but a reality which you admitted to when you identified that there were missing changes. It also means we need to spend more time making sure changes are integrated without introducing issues and finding any missing changes. Finding absent changes was, as an example, a work item that we could have entirely avoided.
  • Significant scope creep — this set of changes was meant to bring prettierX up to date with Prettier. Though additions such as option inheritance were (rightfully) removed due to being out-of-scope, other sets of changes, such as corrections to behaviour not introduced as a result of the merge, should have been dealt with after merging changes and not in this PR. Identifying ways to do things better here in prettierX is an excellent thing to do and worth the time; we handled that correctly at times by creating new issues. However, I don't believe we needed to increase the scope any more past integrating updates and having behaviour maintain with whatever the union of changes from Prettier and prettierX was, priority to the former.
  • Increased workload — this approach of taking a more extensive set of changes and then re-doing them in separate branches and commits effectively doubles the amount of work required to integrate changes. Though we also worked together to smooth out changes, you also worked alone to copy all of that work all over again into multiple different branches. And on top of that, on several occasions, this led to me also having to re-do work to integrate it into branches you created. As you've said, you sacrificed the time needed to introduced badly-needed changes in other OSS projects and delayed getting back to people looking to pay you for part-time work, all just for smaller commits. I do not see this as a trade-off that makes sense.
  • Significant delay — due to the increased workload, the delay with a routine update ballooned to a very long time, given how quickly a code formatter experiences and needs updates. I introduced the original PR prettierx: Merge Prettier 2.3.0 updates - draft 1 #549 43 days ago relative to when you finally merged changes into dev, which at the time of this message is yesterday. I introduced prettierx: Merge Prettier 2.3.1 updates - draft 2 #569 28 days and this PR, Merge additional changes from adalinesimonian/merge-prettier-2.3.1 #603, 19 days before the final merge. In the time since the original PR, Prettier released two patch changes. This delay compared to Prettier's pace of process didn't just add work but shows just how behind we can get if we continue to handle upstream changes in ways with such inefficiency. Unless we are more efficient, I don't have faith in this project keeping up with Prettier long-term without significant overhead or even, potentially, burnout.
  • Less trackable and messy history on GitHub — commits aren't the only place where we keep information on changes. Those changes and the discussions that inform decisions take place in issues, PRs, and discussions (though we do not utilize discussions in this project). While commits may be more granular now, fragmenting changes into multiple branches, PRs, and scope creep has sprawled out the history of this set of changes across the repository. This well undoes any positive benefit of smaller commits. Now, it is more complicated and time-consuming for someone to dig through and locate all the different conversations we and others have had throughout this effort to understand the decision-making process better, the intent behind changes, and understand the scope of those changes. Having clear and organized places to find discussion, I would argue, is far more critical and practical than arbitrarily small commits. Prettier, upstream, doesn't chase small commits. However, because their work is all kept orderly, I was able to dig into history to find reasons for changes and discussion when working on this same set of updates for prettierX. If their commits were smaller, it wouldn't have added any benefit to my search.

This effort to make commits smaller felt like a task that had effort put into it without considering all the other ways in which it was detrimental to the overall effort of updating prettierX, nor the long-term consequences of handling more extensive sets of updates this way. I want to continue to contribute here to prettierX. Still, unless we address this issue of efficiency and complexity, I am not sure I will have the time to devote here and may instead switch to Prettier, where I will have more faith in the continued support of the project.

But I would like to give thanks to you @adalinesimonian for the work on your part, which has enabled this update in the first place, which I think will help many people in the near future.

I truly appreciate it! You're too kind. ☺ It was a lot of work, but together we were able to get it done. At the beginning of this effort, what motivated me to contribute these changes was that prettierX was unusable with Typescript due to lagging behind Prettier. If we can keep this project updated efficiently, it will undoubtedly help adoption. Not everyone has the time like I did to jump in and contribute significant changes, and will instead switch to the upstream project, a consideration that I think makes the argument I make earlier even more important.

@brodycj
Copy link
Owner

brodycj commented Jul 6, 2021

Thanks @adalinesimonian, happy to hear you had a good weekend!

And thanks for the candid feedback. Yes as I said before the work did go through too many circles, especially with the badly-managed scope creep which I should have kept out of this effort in the first place.

Here is what motivated me to in the smaller, more granular commits:

  • I generally rely on the gitx tool on my mac (rowan-gitx from Homebrew). This tool seems to be unrivaled in terms of showing the history of all branches in a repository, which is extremely helpful for dealing with the git forking as in this project. The one thing with gitx is if a diff goes beyond a certain size, it collapses the diff view down to a "click to show diff" thing if a commit is too big, which then takes a moment every time I want to see the changes. This can be pretty disruptive when I am trying to understand and manage things going on in different branches and even different upstream repositories at the same time. I think this may be partly due to the way that gitx watches the git repository on the file system and dynamically updates its GUI upon git updates.
  • I have been thinking about what will happen when I get a chance to rebase all of the prettierX updates on a newer version of the upstream Prettier codebase. My theory (and only a theory) is that the more granular commits should give me an easier and cleaner rebase effort.

I will look for a better way to manage this work over time.

@adalinesimonian adalinesimonian deleted the adalinesimonian/merge-prettier-2.3.1-dev-update-wip branch July 6, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet