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

Display excludeable file types and validate #178

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Conversation

danielingegneri
Copy link
Contributor

@danielingegneri danielingegneri commented Oct 11, 2024

First pull req, plz let me know if needs work. #170

@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 labels Oct 11, 2024
@kehoecj
Copy link
Collaborator

kehoecj commented Oct 11, 2024

@danielingegneri Thanks for the PR! Love the implementation. Looks like there is a goreportcard issue around the complexity in the *excludeFileTypesPtr - please take a look an see if you can resolve.

…nts and flag usages; moved to caller, and made the flags a var block. Unfortunately not able to get complexity below 17.
@danielingegneri
Copy link
Contributor Author

danielingegneri commented Oct 11, 2024

Attempted to reduce complexity. Mostly by simplifying validation more. Also noticed the messages being printed on validation error, and the error being returned, were repeats. Suggest instead displaying the error text in thrown error.
EDIT: Unfortunately can't get complexity below 17 without refactoring more of that function. Complexity seems warranted for the number of variables being validated.

@danielingegneri
Copy link
Contributor Author

Is it preferred that I undo the additional logging changes made, and keep to minimum?

@kehoecj
Copy link
Collaborator

kehoecj commented Oct 21, 2024

Attempted to reduce complexity. Mostly by simplifying validation more. Also noticed the messages being printed on validation error, and the error being returned, were repeats. Suggest instead displaying the error text in thrown error. EDIT: Unfortunately can't get complexity below 17 without refactoring more of that function. Complexity seems warranted for the number of variables being validated.

@danielingegneri I'm willing to push your change for now and deal with refactoring later to get the complexity down a bit. Let me do a final review and if everything looks good I'll merge it.

@kehoecj kehoecj self-requested a review October 21, 2024 15:57
cmd/validator/validator.go Outdated Show resolved Hide resolved
@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 21, 2024
@kehoecj
Copy link
Collaborator

kehoecj commented Oct 23, 2024

@danielingegneri Let me know when this is ready for another review. Looks like a merge conflict and just one comment needs to be resolved before we can merge this in

Copy link
Contributor Author

@danielingegneri danielingegneri left a comment

Choose a reason for hiding this comment

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

I've sorted the list, added space after comma, does look nicer now :)

@kehoecj kehoecj merged commit f3fde7c into Boeing:main Oct 28, 2024
8 of 10 checks passed
@kehoecj kehoecj added OSS Community Contribution Contributions from the OSS Community and removed pr-action-requested PR is awaiting feedback from the submitting developer labels Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest 🎃 Hacktoberfest 2024 hacktoberfest-accepted Valid PR Hacktoberfest PR OSS Community Contribution Contributions from the OSS Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants