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

remove undefined functions in future.h #249

Merged
merged 1 commit into from
Oct 16, 2015

Conversation

kikofernandez
Copy link
Contributor

there are three functions declared in future.h with no definition. they should be removed.

there are three functions declared in `future.h` with no
definition. they should be removed.
@EliasC
Copy link
Contributor

EliasC commented Oct 14, 2015

This seems like a reasonable thing to do. Merging later today.

@albertnetymk
Copy link
Contributor

I am OK with merging this, but it would be better if this kind of “refactoring” could be done along with some real changes so that the PR review process is worth the effort.

@kikofernandez
Copy link
Contributor Author

not sure I agree... what other real change can I add to this PR? I think it's better to have something rather than nothing. I found these by mistake...

  1. should they be in another bigger PR which is not specific to this?
  2. Should I wait until I find more issues similar to this one or I have time to do more refactoring?

I don't like 1 and I am not sure when I will have time to do 2. I think it's better to push a small change that takes 5 min to check and merge.

@EliasC
Copy link
Contributor

EliasC commented Oct 14, 2015

I also think it's better to have these kind of changes in smaller PRs, otherwise it might be lost in review, and I might be looking for these function stubs I left in there 300 commits ago that were covertly removed in some unrelated PR.

Any kind of refactoring that could affect someone else's work (i.e. not whitespace/formatting) could be getting a PR of their own I think.

EliasC added a commit that referenced this pull request Oct 16, 2015
remove undefined functions in `future.h`
@EliasC EliasC merged commit da8b0b9 into parapluu:master Oct 16, 2015
@kikofernandez kikofernandez deleted the fix/undefined-function branch October 16, 2015 14:31
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.

3 participants