Skip to content

Conversation

@yinanazhou
Copy link
Collaborator

  • Fixed crash on loading files

refs: #246

- Update puppeteer to `^19.7.0`
@yinanazhou yinanazhou requested a review from yrammos March 5, 2025 20:19
Copy link
Member

@yrammos yrammos left a comment

Choose a reason for hiding this comment

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

@yinanazhou I sympathize with your interest in prettifying the code… However, I must ask you to maintain the formatting used so far. By changing the formatting of almost every file in the repo, this PR produces countless "nonessential" diffs, and would make it impossible to compare your revisions against past commits….

So, could you either undo the formatting, or adjust your linter to use the same settings as those used in the past?

Sorry about the hassle.

@yinanazhou
Copy link
Collaborator Author

Thanks for the feedback! I totally understand your concern about the nonessential diffs, and I wouldn’t have made these changes if I had found a better way.

Just to clarify, a formatter automatically enforces consistent code style, while a linter primarily checks for potential issues without fixing them. This project uses ESLint as a linter, but there wasn’t a formatter configuration in place before. I also noticed inconsistencies in style both within and across files (e.g., tab size). Since there was no existing standard, introducing a formatter seemed like the easiest way to align styles rather than fixing everything manually.

That said, I’m happy to adjust to any preferred style if you have one, though some changes would be unavoidable. Please let me know how you’d like to proceed.

@yrammos
Copy link
Member

yrammos commented Mar 6, 2025

So, @yinanazhou my suggestion is to revert all formatting changes and keep only the "essential" (i.e. dependency-related) changes.

By the way, issue #246 is specific to this PR, correct? (I don’t think it appears in any other branch of the app). If so, it would be best to keep PR-related bugs as comments within the PR. Does that make sense?

@yrammos
Copy link
Member

yrammos commented Mar 6, 2025

(Alternatively, @yinanazhou if we do open a GitHub issue for a PR-specific thing, it’s best to link the PR and the issue, so that the issue is automatically closed when the PR is merged.)

@yrammos
Copy link
Member

yrammos commented Mar 6, 2025

Looks good, @yinanazhou! Feel free to merge.

@yinanazhou yinanazhou merged commit 4250917 into develop Mar 6, 2025
@yinanazhou yinanazhou deleted the player-fix branch March 6, 2025 22:13
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.

3 participants