some useful updates - Githubissues

airmessage / airmessage-web

AirMessage for the web

https://web.airmessage.org

Apache License 2.0

156 stars 12 forks source link

some useful updates #21

Closed Geczy closed 2 years ago

Geczy commented 2 years ago

check out the commit messages

tagavari commented 2 years ago

Hello, thank you so much for your PR! Before I look into this further, I have a few comments:

Why switch from NPM to PNPM? NPM has been working for me for a long time, and is installed by almost everyone who uses Node.
Can you remove Prettier changes? I want to say thank you for doing this, because code style consistency is important for any project with multiple contributors. However, in this case it makes it difficult for me to compare the changes in this PR. I'd like to address this myself after this PR.

Geczy commented 2 years ago

pnpm is faster, sure npm has been working fine but i don't subscribe to the 'if it aint broke dont fix it' mentality :p

i don't think i can remove the prettier commit. would checking the commits individually help? for example here fa966ad (#21) you can compare the changes pretty easily. i recommend clicking each commit link to see that

or if you want to look at all commits in one pr review, you can skip the prettier commit like this: https://github.com/airmessage/airmessage-web/pull/21/files/b63384d..9776cb0

tagavari commented 2 years ago

Sorry, I'm not interested in adopting pnpm. I'm more familiar with npm and don't see many advantages to switching.

Unfortunately, I cannot merge this with the Prettier commit - it's not possible for me to review all the changes that Prettier made while formatting. I can look into creating a branch with this commit reverted later this week.

Geczy commented 2 years ago

did my last link not help? https://github.com/airmessage/airmessage-web/pull/21/files/b63384d3ecaa6056f9201dedc0131afe08981250..1a46536bb6dff1c35d8d7026bc500ad10725ae77 this omits the prettier commit

tagavari commented 2 years ago

I can see all the changes that you made without the Prettier reformatting, but because it's all grouped under the same branch, I can't merge anything because of all of the files changed by Prettier. There's no reasonable way for me to assess whether the reformatted code is still safe.

Geczy commented 2 years ago

it's safe, it's just formatting. prettier has been around long enough to test these format tools themselves. and i've also been doing this for 15 years. trust me :).

i'm using the app all day since my changes. there's zero bugs due to the formatting.

you're welcome to checkout the branch and test it yourself. not sure why you expect there to be bugs. just because files were changed i can see why you'd be scared. but it's formatting changes, these have been safe to make automatically since 2010. back in 2005 though, they were scary for sure !

bunch more changes coming your way ! whenever you're ready for them i'll open prs here

Geczy commented 2 years ago

you can review just the prettier formatting changes by checking the commit itself here:

https://github.com/airmessage/airmessage-web/pull/21/commits/b63384d3ecaa6056f9201dedc0131afe08981250?w=1

this way you can assess whether the reformatted code is still safe

Geczy commented 2 years ago

opened a new PR with prettier alone