Skip to content

Miscellaneous JavaScript cleanup#4277

Closed
akx wants to merge 16 commits into
gradio-app:mainfrom
akx:js-cleanup
Closed

Miscellaneous JavaScript cleanup#4277
akx wants to merge 16 commits into
gradio-app:mainfrom
akx:js-cleanup

Conversation

@akx

@akx akx commented May 19, 2023

Copy link
Copy Markdown
Contributor

Description

  • relevant motivation: fix possible bugs that are spotted by static analysis tools (PyCharm Pro in this case).
    • a subsequent PR could add e.g. ESlint to catch the rest.
  • a summary of the change: fix those issues. Please see each commit's message for details.

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@gradio-pr-bot

Copy link
Copy Markdown
Collaborator

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4277-all-demos

@akx akx force-pushed the js-cleanup branch 2 times, most recently from d380690 to fe8a476 Compare May 19, 2023 09:06
@akx akx marked this pull request as ready for review May 19, 2023 09:30
@pngwn

pngwn commented May 20, 2023

Copy link
Copy Markdown
Member

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 pngwn closed this May 20, 2023
@akx

akx commented May 20, 2023

Copy link
Copy Markdown
Contributor Author

@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? 😁)

it is unfortunately a bad time for this

How would a contributor know when it's a good time to contribute?

@abidlabs

Copy link
Copy Markdown
Member

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.

@akx

akx commented May 20, 2023

Copy link
Copy Markdown
Contributor Author

There honestly are no major changes in this PR if you look at it commit by commit (atomic commits for the win).
I can split this into smaller PRs (atomic commits for the win, again) if that helps.

@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... 😁

@pngwn

pngwn commented May 20, 2023

Copy link
Copy Markdown
Member

How would a contributor know when it's a good time to contribute?

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.

@akx

akx commented May 20, 2023

Copy link
Copy Markdown
Contributor Author

@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).

@pngwn

pngwn commented May 20, 2023

Copy link
Copy Markdown
Member

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.

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.

4 participants