Skip to content

Comments

docs: update coverage_excludes example to something more useful#323

Merged
KyleFin merged 1 commit intoVeryGoodOpenSource:mainfrom
juandelgado:patch-1
Mar 20, 2025
Merged

docs: update coverage_excludes example to something more useful#323
KyleFin merged 1 commit intoVeryGoodOpenSource:mainfrom
juandelgado:patch-1

Conversation

@juandelgado
Copy link
Contributor

@juandelgado juandelgado commented Mar 20, 2025

Status

READY

Description

While the current *.g.dart example is technically valid, a more useful example would be something like **/**.g.dart.

The current example only excludes generated files in the root directory where they're rarely placed.

The updated **/*.g.dart will cover generated files anywhere in the repository, which is probably a lot more helpful in real life scenarios.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@juandelgado juandelgado requested a review from a team as a code owner March 20, 2025 10:39
alestiago
alestiago previously approved these changes Mar 20, 2025
uses: VeryGoodOpenSource/very_good_workflows/.github/workflows/flutter_package.yml@v1
with:
coverage_excludes: '*.g.dart'
coverage_excludes: '**/**.g.dart'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
coverage_excludes: '**/**.g.dart'
coverage_excludes: '**/*.g.dart'

I think there is a subtle difference and **/*.g.dart might hit more scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's rabbit hole I didn't want to fall into 😅

I did look at the implementation and under the hood minimatch is being used.

My guess is that **/**.g.dart being a bit more eager and catching more scenarios should be a good thing? My head is at "every generated file anywhere should be ignored", but that might be me oversimplifying things.

Happy with either option!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should use whichever is more eager.

I'm not finding examples of patterns ending in /**.ending. I believe ** is more for matching arbitrary directory depth (so it makes more sense to use it in **/, /**/, or /**)

I believe we should use **/*.g.dart here, but not totally sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in for the more eager option!

From the small research I did I found **/*.g.dart was more eager than **/**.g.dart too.

Copy link
Contributor Author

@juandelgado juandelgado Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, gimme 5, I'll update it.

EDIT: done.

Thanks!

KyleFin
KyleFin previously approved these changes Mar 20, 2025
While the current `*.g.dart` example is technically valid, a more useful example would be something like `**/*.g.dart`.

The current example only excludes generated files in the root directory where they're rarely placed.

The updated `**/*.g.dart` will cover generated files anywhere in the repository, which is probably a lot more helpful to real life scenarios.
@juandelgado juandelgado dismissed stale reviews from KyleFin and alestiago via d5cac29 March 20, 2025 17:10
@KyleFin KyleFin merged commit 7131cfc into VeryGoodOpenSource:main Mar 20, 2025
9 checks passed
juandelgado added a commit to juandelgado/kagi-test that referenced this pull request May 19, 2025
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.

3 participants