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

Revert "Update of codi and medi" #667

Closed
wants to merge 1 commit into from

Conversation

pcarruscag
Copy link
Member

Proposed Changes

@oleburghardt, according to @talbring this was supposed to go AFTER #653. Is my work a joke around here or what?!

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.

@oleburghardt
Copy link
Contributor

oleburghardt commented Apr 4, 2019

@oleburghardt, according to @talbring this was supposed to go AFTER #653. Is my work a joke around here or what?!

Sorry, haven't been aware of that. Just had a look at the code which looked good to me.

@rsanfer
Copy link
Contributor

rsanfer commented Apr 4, 2019

Ok, so it was a mistake, which is exactly why version control is in place. Let's keep it calm, we can fix it.

@talbring
Copy link
Member

talbring commented Apr 4, 2019

@pcarruscag You are absolutely right in that I said we should merge both of your PRs in first. I know that it is sometimes frustrating when it takes more time than it should, before we (or I) can do a review ...

@economon
Copy link
Member

economon commented Apr 4, 2019

Yep, just an honest mistake. We'll fix it up.

I know that things have been a little quiet in the repository lately, but there is a lot of motion happening behind the scenes as we prepare for the developers meeting next month (we have some exciting things in store).

Thanks for the patience, and I would also ask that, if folks in the community have some time, they please contribute to reviews. Expertise in the particular area is not required (I know it can seem intimidating, but don't be shy!), and it is a great way to learn the code and see what other folks are developing. The more input and discussion we have from various perspectives, the better.

@talbring
Copy link
Member

talbring commented Apr 4, 2019

My suggestions: Let's keep the other PR merged and we close this one. @pcarruscag I will deal with the conflicts in your branch now. It's also a good opportunity for me to look at the changes ;)

@oleburghardt
Copy link
Contributor

Thanks at all for being so responsive to this mishap.

When I started contributing I learned that something like a 2-LGTM-rule was applying. But apparently it evolved to have someone merge a pull request if he or she can judge the content and feels comfortable with it, as the other approach ended up having a large list of unmerged pull requests or having two LGTM's of non-independent reviewers.

@economon Maybe you can bring it up at the next meeting how we could address this little double bind?

So sorry again for the trouble (at least a revert of the very latest commit would not be too difficult). Still I'll wait if @pcarruscag and @talbring want to do now the way Tim suggested.

@pcarruscag
Copy link
Member Author

I'll fix my branch. If you do not have time for reviews I am not going to ask you to fix my code.

@pcarruscag pcarruscag closed this Apr 4, 2019
@pcarruscag pcarruscag deleted the revert-660-update_codi_medi branch February 26, 2020 19:56
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.

5 participants