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

Updated URL in references to Cantera's license. #709

Merged
merged 1 commit into from Sep 24, 2019

Conversation

paulblum
Copy link
Contributor

@paulblum paulblum commented Sep 19, 2019

Changes proposed in this pull request:

@codecov
Copy link

codecov bot commented Sep 19, 2019

Codecov Report

Merging #709 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #709      +/-   ##
==========================================
- Coverage   70.63%   70.63%   -0.01%     
==========================================
  Files         372      372              
  Lines       43567    43567              
==========================================
- Hits        30773    30772       -1     
- Misses      12794    12795       +1
Impacted Files Coverage Δ
include/cantera/thermo/EdgePhase.h 100% <ø> (ø) ⬆️
src/thermo/GibbsExcessVPSSTP.cpp 75.47% <ø> (ø) ⬆️
src/clib/ctsurf.cpp 0% <ø> (ø) ⬆️
include/cantera/kinetics/ImplicitSurfChem.h 100% <ø> (ø) ⬆️
src/thermo/MixtureFugacityTP.cpp 28.77% <ø> (ø) ⬆️
include/cantera/thermo/NasaPoly1.h 92.18% <ø> (ø) ⬆️
include/cantera/transport/TransportBase.h 7.69% <ø> (ø) ⬆️
include/cantera/cython/funcWrapper.h 82.6% <ø> (ø) ⬆️
src/thermo/HMWSoln.cpp 71.54% <ø> (ø) ⬆️
include/cantera/thermo/PDSS_Water.h 100% <ø> (ø) ⬆️
... and 211 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 265a186...97bc484. Read the comment docs.

@ischoegl
Copy link
Member

ischoegl commented Sep 19, 2019

While this PR does address a known issue, it will create quite a few merge conflicts for pending pull requests (plus complete re-builds for existing toolchains when switching between branches that are not rebased). If adopted, I'd request to make this change at the last moment prior to the official release of 2.5.

@bryanwweber
Copy link
Member

bryanwweber commented Sep 19, 2019

@ischoegl I hope this doesn't create many merge conflicts. We don't have many open PRs that touch the top of these files, I think yours are the only ones, so it should be pretty easy to sort out.

For the other concern, I'm afraid there's not much we can do about that... Any long-running branches will have to rebased onto master before they're merged anyways, so they might as well be rebased after (if) this change is merged so that the rebuild only happens once. I don't see why waiting until just before 2.5 would help that much, could you clarify that?

@ischoegl
Copy link
Member

ischoegl commented Sep 19, 2019

@bryanwweber I haven't checked other PR's, but there are indeed quite a few files that are affected in what is currently pending for myself. It's not a big issue, so I"ll deal with it. Regarding my comment: a version jump appears to be the obvious time when house-keeping operations are applied.

@ischoegl
Copy link
Member

ischoegl commented Sep 20, 2019

@paulblum ... thank you submitting the PR regardless of my above comment(s). Depending on the order of approval of pending PR's, we'll just have to resolve the unavoidable merge conflicts case-by-case. Overall, it's a good thing if this is taken care of once and for all.

@speth
Copy link
Member

speth commented Sep 20, 2019

@ischoegl Just having changes in the same file won't generate merge conflicts -- they need to be changes to the same line or the two adjacent lines in order for git's automatic merge algorithm to fail and require human intervention. If the two branches make the exact same changes, the merge/rebase will also succeed. Of your 6 open PRs, the only place where I get a merge conflict when rebasing is for one file (base.pyx) in #696, since you have additions immediately below the URL being updated here.

I understand this isn't completely painless, but I had suggested making this change en masse to avoid having to keep asking PR authors to make the piecemeal version of this change and to keep this unrelated change out of the diff of future commits / PRs.

@ischoegl
Copy link
Member

ischoegl commented Sep 20, 2019

@speth ... thanks for the clarification. I apparently did not recognize that identical changes in independent branches won't cause problems, despite this being the logical behavior. I apologize if dread of having to perform rebase-acrobatics got the better of me.

@bryanwweber
Copy link
Member

bryanwweber commented Sep 20, 2019

@ischoegl Rebase-acrobatics-dread is certainly understandable. I'm reminded of this XKCD comic: https://xkcd.com/1597/ As @speth said, this touches areas of the files unlikely to be touched in other branches. I also wasn't aware that git would just figure things out if the same change was made twice, but I'm glad to know that's the case!

Also, full disclosure, @paulblum is one of my students and I asked him to make this PR 😸

@ischoegl
Copy link
Member

ischoegl commented Sep 20, 2019

@bryanwweber ... thanks for the XKCD link - he always seems to have an appropriate comic! Glad to have learned something useful also: no more automatic alarm bells if the same lines are touched in separate branches ... (still: beware of adjacent modifications!)

@paulblum In hindsight my comments were based on a false alarm on my part. Sorry about that ...

@speth
Copy link
Member

speth commented Sep 24, 2019

To add a little data to the theory: After having merged #689, which makes a this same update to several files, Github still says "Rebase and merge can be performed automatically." So with that, I think this is safe to merge.

@speth speth merged commit c4aff04 into Cantera:master Sep 24, 2019
3 of 4 checks passed
@paulblum paulblum deleted the urlupdate branch Feb 26, 2021
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.

None yet

4 participants