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.

Sign up for GitHub

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

Jump to bottom

add file-types flag #192

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

add file-types flag #192

wants to merge 15 commits into from

Conversation

gurukiran07
Copy link
Contributor

@gurukiran07 gurukiran07 commented Oct 25, 2024 •
edited
Loading

closes #174

Added file-types flag to validator. file-types and exclude-file-types are mutually exclusive. if file-type is set then builds a list of files that can be excluded and sets that value to exclude-file-types flag.

Some rudimentary tests:

with --file-types=yaml
image

with --exclude-file-types and --file-types
image

@gurukiran07 gurukiran07 mentioned this pull request Oct 25, 2024
Open
@kehoecj kehoecj added hacktoberfest-accepted Valid PR Hacktoberfest PR hacktoberfest 🎃 Hacktoberfest 2024 waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 25, 2024
@kehoecj kehoecj self-requested a review October 25, 2024 14:50
kehoecj
kehoecj requested changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

  1. Add unit tests for your changes
  2. Fix issues identified in failed jobs: golangci-lint and goreportcard

cmd/validator/validator.go Outdated Show resolved Hide resolved
@kehoecj kehoecj added waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers and removed pr-action-requested PR is awaiting feedback from the submitting developer labels Oct 25, 2024
@gurukiran07
Copy link
Contributor Author

  1. Add unit tests for your changes
  2. Fix issues identified in failed jobs: golangci-lint and goreportcard

Done. Pipeline's green. Let me know if there's anything else I need to do. @kehoecj

@kehoecj
Copy link
Collaborator

kehoecj commented Oct 28, 2024

@gurukiran07 Please resolve conflicts then I think you're good to go

@kehoecj kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 28, 2024
@gurukiran07
Copy link
Contributor Author

@gurukiran07 Please resolve conflicts then I think you're good to go

Done.

ccoVeille
ccoVeille reviewed Oct 28, 2024
View reviewed changes
cmd/validator/validator.go Outdated Show resolved Hide resolved
cmd/validator/validator.go Outdated Show resolved Hide resolved
cmd/validator/validator.go Outdated Show resolved Hide resolved
cmd/validator/validator.go Outdated Show resolved Hide resolved
cmd/validator/validator.go Outdated Show resolved Hide resolved
@gurukiran07
Copy link
Contributor Author

Will make the requested changes when I get some time today.

@kehoecj
Copy link
Collaborator

kehoecj commented Oct 30, 2024

@gurukiran07 Please resolve conflicts and I'll work on getting this merged today

@gurukiran07
Copy link
Contributor Author

@gurukiran07 Please resolve conflicts and I'll work on getting this merged today

@kehoecj Done. Fixed the conflicts.

Some basic testing:

  • with both exclude-file-types and file-types
image
  • with just file-types
image
  • with just exclude-file-types
image

ccoVeille
ccoVeille reviewed Oct 30, 2024
View reviewed changes
cmd/validator/validator.go Outdated Show resolved Hide resolved
cmd/validator/validator.go Outdated Show resolved Hide resolved
cmd/validator/validator.go Outdated
Comment on lines 338 to 352
for _, t := range filetype.FileTypes {
validTypes = append(validTypes, t.Name)
}

validExcludeTypes := misc.ArrToMap(validTypes...)

for _, t := range strings.Split(*fileTypesPtr, ",") {
delete(validExcludeTypes, t)
}

excludeFileTypes := make([]string, 0, len(validExcludeTypes))

for fileType := range validExcludeTypes {
excludeFileTypes = append(excludeFileTypes, fileType)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks complicated

Isn't something that could be used in Go standard "slice" package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't anything useful in slices packages that can be used here. Would adding comments make it more readable? @ccoVeille

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed - I think this can be simplified a bit

Copy link
Contributor Author

@gurukiran07 gurukiran07 Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Refactored it to use slices.DeleteFunc.
@ccoVeille @kehoecj

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, as you can see the code is now way clearer

ccoVeille
ccoVeille reviewed Oct 30, 2024
View reviewed changes
cmd/validator/validator.go
@@ -162,6 +168,10 @@ Supported formats: standard, json, junit (default: "standard")`,
}
}

if err := buildExcludeFileTypesFromFileTypes(excludeFileTypesPtr, fileTypesPtr); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW @kehoecj is there a way to validate the behavior of exclusive exclusion (cannot use filter + exclusion) for types in the tests?

Or the code needs to be refactored a lot?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be possible to validate in testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I think it should be part of the PR

kehoecj
kehoecj reviewed Oct 30, 2024
View reviewed changes
cmd/validator/validator.go Outdated Show resolved Hide resolved
ccoVeille
ccoVeille reviewed Nov 2, 2024
View reviewed changes
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Do you think you could add the tests I'm talking about here

#192 (comment)

@gurukiran07
Copy link
Contributor Author

Do you think you could add the tests I'm talking about here

#192 (comment)

Yeah I can. Will work on it when I get some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccoVeille ccoVeille ccoVeille left review comments

@kehoecj kehoecj Awaiting requested review from kehoecj

Assignees
No one assigned
Labels
hacktoberfest 🎃 Hacktoberfest 2024 hacktoberfest-accepted Valid PR Hacktoberfest PR pr-action-requested PR is awaiting feedback from the submitting developer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add --file-types flag to include only specified config file types
3 participants
@gurukiran07 @kehoecj @ccoVeille