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

Use the options pattern #101

Merged
merged 20 commits into from
Dec 10, 2024
Merged

Use the options pattern #101

merged 20 commits into from
Dec 10, 2024

Conversation

MetalBlueberry
Copy link
Contributor

@MetalBlueberry MetalBlueberry commented Dec 3, 2024

Using the options pattern makes a clean interface for the package that will be easier to maintain in the long run.

csvcopy.WithReportingPeriod(reportingPeriod),
csvcopy.WithVerbose(verbose),
}
if headerLinesCnt > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be:

Suggested change
if headerLinesCnt > 1 {
if headerLinesCnt > 0 {

Otherwise you'll enter the fail branch of WithSkipHeaderCount:

if headerLineCount == 0 {
			return errors.New("header line count must be greater than zero")
}

Another thing, this changes the current behavior. I'm not saying the pass behavior is the best either. The old behavior is, if skip == false then it doesn't matter the value of headerLinesCnt, nothing is going to be skipped. By default headers are not skipped.

With you changes, since headerLinesCnt defaults to 1, and you are not requiring skip == true to set the value for skip, then we are defaulting to always skip the first line.

I'd rather not change the old behavior, and if we do it, let's document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point, I was excited about the change and decided that this behavior was a bit better. But there is no real reason to change it. So I'm going to leave it as it was.

I'm going to leave the option to just set skip header, as that is the most common configuration and the one we will be using on our system.

@MetalBlueberry MetalBlueberry marked this pull request as ready for review December 10, 2024 17:56
@MetalBlueberry MetalBlueberry merged commit 464a7be into main Dec 10, 2024
3 checks passed
@MetalBlueberry MetalBlueberry deleted the vperez/use-options-pattern branch December 10, 2024 21:10
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.

2 participants