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

Revert contructor arguments #703

Merged
merged 1 commit into from
Jul 8, 2022
Merged

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Jul 8, 2022

This PR reverts #442 (7ac17d0)

This patch was ported to internal in cl/449707130, but benchmarking
revealed that the new constructors add to release binary sizes even when
no arguments are passed to the constructors.

To keep the two code bases in sync I'm reverting this change.

These new constructor arguments were never announced in the changelog,
but I still added an entry to show how to migrate code that uses them.

@osa1 osa1 requested a review from mraleph July 8, 2022 16:38
@osa1 osa1 force-pushed the revert_constructor_args branch 3 times, most recently from 5bd7742 to fb37cd5 Compare July 8, 2022 16:42
This PR reverts google#442 (7ac17d0)

This patch was ported to internal in cl/449707130, but benchmarking
revealed that the new constructors add to release binary sizes even when
no arguments are passed to the constructors.

To keep the two code bases in sync I'm reverting this change.

These new constructor arguments were never announced in the changelog,
but I still added an entry to show how to migrate code that uses them.
@osa1 osa1 force-pushed the revert_constructor_args branch from fb37cd5 to d973126 Compare July 8, 2022 16:45
@osa1 osa1 merged commit 2d6c603 into google:master Jul 8, 2022
@osa1 osa1 deleted the revert_constructor_args branch July 8, 2022 17:02
@agreaves
Copy link
Contributor

agreaves commented Jun 18, 2023

I know this is closed, but I'd like to voice the opinion that this is an incredibly harmful change for code quality. The constructors provide much needed structure and given Dart's formatting, the cascading Foo()..bar = 4 approach is unwieldily and leads to far more unreadable code.

For this reason, we use named constructor arguments heavily in our codebase and this sole change is likely the reason we won't update the proto libraries any time soon. We'd have to undergo a painful migration just to have worse code quality in the end. In our case at least, the binary size increase is negligible in comparison. Realistically, though, the most likely scenario is that we fork the repo and revert this change (which will be no picnic either given future changes).

At the very least, I'd recommend making it optional and adding a flag for whether to add constructors args in the generated code (defaulting to false). If truly the only downsize is a larger binary I really think it's worth keeping at least as an option.

@agreaves
Copy link
Contributor

Actually, I'll just create an issue for this for visibility. Missed in all of the above is a big thank you for all the hard work on this library! It's actually extremely flexible and nice to use! Perhaps that's why I'm so passionate about my complaints 😅

@iulian0512
Copy link

this is a major breaking change

@LukaszRacon
Copy link

Issue created by @agreaves: #850

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.

5 participants