dart-lang / mockito

Mockito-inspired mock library for Dart
https://pub.dev/packages/mockito
Apache License 2.0
632 stars 162 forks source link

Make MockBuilder support build_extensions option. #685

Closed LuisDuarte1 closed 1 year ago

LuisDuarte1 commented 1 year ago

This PR changes the behaviour of the buildMocks factory, in order to support the build_extensions option.

This is useful in order to change the destination directory of the mocks, allowing for more customizability as in this article.

This change is a bit opinionated, because it forces the end-user to have a output filename pattern that ends with .mocks.dart in order to be consistent with the already generated files from mockito. It also forces the user, to have an input pattern ending with .dart.

Closes https://github.com/dart-lang/mockito/issues/545


Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
yanok commented 1 year ago

Corr: actually example/ is not a separate project, so maybe just change the top-level build.yaml file to show how it works?

LuisDuarte1 commented 1 year ago

LGTM, but we need:

  1. Some documentation update to mention this feature, maybe even a FAQ entry;
  2. Maybe add build_extensions to example/ project?

I agree so I've added them.

Also, I found some edge cases with my implementation so I took care of them, I'll list them:

natebosch commented 1 year ago

and squash all commits into one?

We should be able to squash for the internal commit - generally it's easier to avoid force-pushing over shared history in a PR.

LuisDuarte1 commented 1 year ago

and squash all commits into one?

We should be able to squash for the internal commit - generally it's easier to avoid force-pushing over shared history in a PR.

Well didn't read that in time, sorry ^^ should I revert it then?

natebosch commented 1 year ago

should I revert it then?

No need - we can work with it either way. I was just hoping to inform on best practices since I frequently use shared PR history when reviewing in dart-lang reps 😄

yanok commented 1 year ago

We should be able to squash for the internal commit - generally it's easier to avoid force-pushing over shared history in a PR.

Well, actually Copybara already does the squash, so it's mostly about getting a nicer CL description, as now it will list all the commits. We should probably just fix our Copybara config to copy the PR description into CL.

yanok commented 1 year ago

We need to wait for the resolution of the discussion above. Also please convert the added double-quotes in the test file into single quotes: we have a lint complaining about that internally.

yanok commented 1 year ago

Now the formatter complains :) Could you please re-format it?

LuisDuarte1 commented 1 year ago

Now the formatter complains :) Could you please re-format it?

Done. It seems that Dart 3.0 has a different formatting than Dart 2.19, a quick setup in github codespaces allowed me to format the files in the CI version :sweat_smile:.

yanok commented 1 year ago

Done. It seems that Dart 3.0 has a different formatting than Dart 2.19, a quick setup in github codespaces allowed me to format the files in the CI version 😅.

Arghh, I see. Yes, I think we did change the formatter some time ago. Damn, then we should have just bumped the required SDK version I guess... I wonder if now formatter will start complaining internally, since we use more or less HEAD version... But let's wait.

yanok commented 1 year ago

Nope, it worked fine, thanks!