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

[General behavior] Migrate Widget methods to classes #2621

Open
Tracked by #1712
g123k opened this issue Jul 18, 2022 · 5 comments
Open
Tracked by #1712

[General behavior] Migrate Widget methods to classes #2621

g123k opened this issue Jul 18, 2022 · 5 comments

Comments

@g123k
Copy link
Collaborator

g123k commented Jul 18, 2022

What

  • By fixing issue Can't remove single products from carousel #2603, I have discovered the summary_card file, where a single Widget contains 700 lines of code! 😱
    This is generally a bad coding behavior for Flutter, knowing how the different trees work.
  • I think we should take some time to go through the code and split this kind of Widgets.
  • For the first step, I suggest listing all files and they start fixing them gradually.

Part of

@monsieurtanuki
Copy link
Contributor

monsieurtanuki commented Jul 18, 2022

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

Kind of a subtask ;) but thanks for the list of files @monsieurtanuki!

@monsieurtanuki
Copy link
Contributor

Cool! I suggest that you (or whoever) do that explicitly, one by one, by creating a specific issue for each file like "summary_card.dart refactoring" issue:

  • it can be very painful to review that kind of code if too many files are involved, and very frustrating for the developer to wait very long for a review
  • I don't think it's a priority task - so let's not fall into the "purest code" rabbit hole while we have major issues pending (like offline or FDroid)
  • it could be helpful for other developers to answer "hey, I'm actually working on this file, so let me do it / please do it fast / please do it later / I'll code another issue instead"

@g123k
Copy link
Collaborator Author

g123k commented Jul 18, 2022

@monsieurtanuki
Copy link
Contributor

Then I would encourage you to create a PR that just adds one TODO on top of each suspicious file (a dozen I guess), something like // TODO(g123k): if you're working on that file and if you have time, please split that file / that widget into smaller bits (e.g. less than 300 lines).
This way, next time any developer that edits one big file will be aware. And in the meanwhile everybody works on higher priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 💬 To discuss and validate
Development

No branches or pull requests

3 participants