Feature/merge - Githubissues

ethz-asl / curves

A library of curves for estimation.

BSD 3-Clause "New" or "Revised" License

74 stars 28 forks source link

Feature/merge #59

Closed pfankhauser closed 7 years ago

ethzasl-jenkins commented 7 years ago

Can one of the admins verify this patch?

pfankhauser commented 7 years ago

add to whitelist

pfankhauser commented 7 years ago

ok to test

pfankhauser commented 7 years ago

test this please

pfankhauser commented 7 years ago

Thanks a lot @rdube! I started implementing some of your suggestions, but before we go all the way we should discuss the following points:

Variables names style,
Braces position,
Line width.

pfankhauser commented 7 years ago

I realize we still have lots of work to do here, but I suggest we go ahead and merge this PR into master and put it on public. @rdube and @gawela could you please tell us when we can do this? For us, the sooner the better.

rdube commented 7 years ago

@pfankhauser IMO you are good to go. Wait for the LGTM of @gawela though.

gawela commented 7 years ago

@pfankhauser I'd like to give it a pass this afternoon, if that's ok.

Regarding the style: I'd prefer to use the usual Google style guide, but we can fix this after merge.

pfankhauser commented 7 years ago

Thanks a lot guys for your reviews! I think you all good very valid points. Since curves grew historically it really needs some more cleanups. Since this PR is becoming huge I suggest the following:

From the most important discussion points I created separate issues, we can discuss on these separately and have a meeting to take a decision,
To accelerate the progress I'll merge this PR and set it on public,
We'll continuously work together to re-add missing features and follow up with the cleanups.

Do you guys agree?

rdube commented 7 years ago

@gawela nice review! :) @pfankhauser go for it!!

rdube commented 7 years ago

w00t w00t!!