Miscellaneous JavaScript cleanup#4277
Conversation
|
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4277-all-demos |
d380690 to
fe8a476
Compare
|
Appreciate the effort that has gone into this PR but it is unfortunately a bad time for this. Quite a lot of these are stylistic and of limited value (if any) but this PR introduces too many changes at a time when there will be considerable changes in the future. I don't think any of these changes prevent of fix any bugs. We'll add eslint in the future. |
|
@pngwn Did you read through them? E.g. the TODO comments where I marked possible bugs? (Or where awaits are being blatantly missed?) (Also, can you point towards the changes that don't have any value? 😁)
How would a contributor know when it's a good time to contribute? |
|
I'll let @pngwn speak to the specifics of this PR, but generally we ask contributors to create issues before creating PRs. For small PRs, it's not a big deal, but this allows maintainers to comment on whether this is a good time to contribute towards the issue. In this case, the frontend is getting a sizeable refactor as we work towards 4.0 so this is not the best time for major changes. |
|
There honestly are no major changes in this PR if you look at it commit by commit (atomic commits for the win). @abidlabs 👍 Thank you for the information – where is the refactoring for 4.x being done (and/or is there an ETA or roadmap for it)? There are 119 branches in this repo according to GitHub, so it's hard to tell... 😁 |
As @abidlabs said, we prefer every PR to be accompanied by an issue. Especially for large PRs and anything that requires significant effort. This allows us to discuss the changes ahead of any work being started. |
|
@pngwn I don't think it makes much sense to create an issue that says "this code could be optimized and have less possible corner case issues arising from missed awaits" etc., and either way, while I understand the preference, there's a whole bunch of PRs in this repo that don't have related issues. I'd like to know if this was even considered to be reviewed at all, or just categorically closed as "nah, it's a bad time or something"? Sure, some of the commits are stylistic (e.g. using template strings wherever possible) and could be snipped out if requested. As I said in the earlier comment, this is really not a big PR (it's approximately the same size as the recently merged #4256). |
|
I did review it, yes. This discussion isn't really going anywhere. We've explained our position and how we do things, you don't have to agree of course, but from our perspective drive-by PRs are rarely helpful. |
Description
Checklist: