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

Factor out initial infections #318

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Factor out initial infections #318

wants to merge 2 commits into from

Conversation

zebmason
Copy link
Contributor

No description provided.

CloneDeath
CloneDeath approved these changes May 21, 2020
View reviewed changes
Copy link
Contributor

@CloneDeath CloneDeath left a comment

Choose a reason for hiding this comment

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

I'd like the see the mentioned changes (all pretty minor), but otherwise I definitely approve of this change.

src/Param.h Show resolved Hide resolved
src/Param.h Outdated Show resolved Hide resolved
src/Param.h Outdated Show resolved Hide resolved
matt-gretton-dann
matt-gretton-dann reviewed May 21, 2020
View reviewed changes
Copy link
Collaborator

@matt-gretton-dann matt-gretton-dann left a comment

Choose a reason for hiding this comment

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

At the moment this feels like more typing for not much benefit. What is the end goal of this change? Where do you see this heading?

@zebmason
Copy link
Contributor Author

@matt-gretton-dann Just initial grouping of logically related values so as to reduce the number of elements in params.

@zebmason
Copy link
Contributor Author

@matt-gretton-dann Perhaps I should have mentioned that I've been further developing my code analysis tool to factor out classes from dependencies so my commits since I stopped playing with GoogleTest (I've made it safe for CI by commenting out) have mainly been the result of testing the new tool.

@zebmason zebmason force-pushed the initial_infections branch from d60eab1 to 6471021 Compare June 5, 2020 06:01
@zebmason zebmason marked this pull request as draft June 25, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matt-gretton-dann matt-gretton-dann matt-gretton-dann left review comments

@CloneDeath CloneDeath CloneDeath approved these changes

@dlaydon dlaydon Awaiting requested review from dlaydon dlaydon is a code owner

@gnedjati gnedjati Awaiting requested review from gnedjati gnedjati is a code owner

@igfoo igfoo Awaiting requested review from igfoo igfoo is a code owner

@NeilFerguson NeilFerguson Awaiting requested review from NeilFerguson NeilFerguson is a code owner

@Z80coder Z80coder Awaiting requested review from Z80coder Z80coder is a code owner

@weshinsley weshinsley Awaiting requested review from weshinsley weshinsley will be requested when the pull request is marked ready for review weshinsley is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zebmason @CloneDeath @matt-gretton-dann