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

add AddSpecialSuiteFailureReasons to reduce boilerplate #1411

Closed
wants to merge 1 commit into from

Conversation

grosser
Copy link
Contributor

@grosser grosser commented May 9, 2024

basically

report.SpecialSuiteFailureReasons = append(report.SpecialSuiteFailureReasons, suite.CompilationError.Error())
->
report.AddSpecialSuiteFailureReasons(suite.CompilationError.Error())

because the boilerplate was a bit annoying to see everywhere and now the lines are easier to read

@onsi
Copy link
Owner

onsi commented May 21, 2024

heyo - i appreciate the boiler plate is annoying but I'd prefer to keep it as-is for now. the report isn't a pointer and i'm trying to be more explicit about when and how it changes.

@grosser
Copy link
Contributor Author

grosser commented May 21, 2024

FYI I started with keeping just 1 method as pointer and that felt weird :)
I like the idea of making it a pointer since I don't like dealing with some things being a pointer and others not, but yeah if you have other plans 🤷
... feel free to close if you don't think this is mergeable in the near future

@onsi
Copy link
Owner

onsi commented May 21, 2024

yeah i hear you. a lot of the mess in v1 stemmed form me using pointers everywhere and not managing state mutation more carefully. v2 tried to take a more immutable pov (though, ultimately, there are some pointers here or there). it definitely helped avoid some of the issues i hit with v1 though it's generated it's own kinds of messes along the way 🤷

@onsi onsi closed this May 21, 2024
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