Skip to content

Add globbing of files for linting#449

Merged
daveshanley merged 6 commits intodaveshanley:mainfrom
volovikariel:add-globbing-of-files-for-linting
Feb 14, 2024
Merged

Add globbing of files for linting#449
daveshanley merged 6 commits intodaveshanley:mainfrom
volovikariel:add-globbing-of-files-for-linting

Conversation

@volovikariel
Copy link
Copy Markdown
Contributor

@volovikariel volovikariel commented Feb 8, 2024

This is adds the globbing feature very shortly discussed here: #406 (specifically, here)

It allows for typing something like this vacuum lint --globbed-files="*/**/bu*.openapi.yaml" model/test_files/badref-burgershop.openapi.yaml, which will glob files relative to the current working directory.

It is also possible to combine globbing with the current way of specifying files (just providing filepath(s)), it removes duplicates (meaning, if you provide a filepath twice, or if one of your filepaths provided is also a match result of globbing).

This is my first time actually contributing code to an open source project - so, I'm expecting quite a lot of feedback/criticism. I'm eager to learn!

@daveshanley daveshanley added the release/patch Patch / non-breaking release label Feb 8, 2024
Comment thread cmd/lint.go Outdated
Comment thread cmd/lint.go Outdated
Comment thread cmd/lint.go Outdated
Comment thread cmd/lint.go Outdated
Comment thread cmd/lint.go Outdated
Comment thread cmd/lint.go Outdated
Comment thread cmd/lint.go Outdated
Copy link
Copy Markdown
Owner

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

LGTM!

The only other suggestion I have is to add in a TODO that mentions this new functionality needs to be added to other commands as well at some point.

@daveshanley
Copy link
Copy Markdown
Owner

Would you be able to rebase?

@volovikariel
Copy link
Copy Markdown
Contributor Author

Would you be able to rebase?

Don't totally understand why. Is it to avoid merge commits in the history?

If so - should I squash all of the commits together into a single commit with message "add globbing of files for linting (#406)"?
This would be to avoid commits that have a "broken state" (like my first "checkpoint" commit), for if we ever want to run git bisect to check for some regression.

@daveshanley
Copy link
Copy Markdown
Owner

daveshanley commented Feb 13, 2024

Would you be able to rebase?

Don't totally understand why. Is it to avoid merge commits in the history?

If so - should I squash all of the commits together into a single commit with message "add globbing of files for linting (#406)"? This would be to avoid commits that have a "broken state" (like my first "checkpoint" commit), for if we ever want to run git bisect to check for some regression.

I prefer rebasing to merging for one reason, timeline preservation. because I am the primary contributor to this codebase, I like to see 'blocks' of feature time on my repo timeline.

When we merge, it blends the timeline and I lose track of which author did what bit when. rebasing keeps your contributions cleanly blocked into a segment.

Not every single commit will always build, thats OK, as long as the main branch is always 100% operational.

There is no need to squash, I don't like squashing except for nit commits and cosmetic stuff.

@volovikariel volovikariel force-pushed the add-globbing-of-files-for-linting branch 2 times, most recently from b1f4877 to ac3262d Compare February 13, 2024 14:33
@volovikariel
Copy link
Copy Markdown
Contributor Author

volovikariel commented Feb 13, 2024

I think it should be what you wanted now 🤔

Went through quite a rollercoaster (as you can tell by the many force pushes...) but - the merge commit is gone now 🥳

@volovikariel volovikariel force-pushed the add-globbing-of-files-for-linting branch from 3a2d08c to eddf94f Compare February 13, 2024 15:01
@daveshanley daveshanley merged commit 62ab5d0 into daveshanley:main Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release/patch Patch / non-breaking release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants