Skip to content

Conversation

@Diapolo
Copy link

@Diapolo Diapolo commented Sep 9, 2014

  • add missing header end comments
  • ensure alphabetical ordering
  • update copyright year and license

@Diapolo Diapolo mentioned this pull request Sep 9, 2014
@jtimon
Copy link
Contributor

jtimon commented Sep 9, 2014

I was only fixing the makefile in #4755 and @TheBlueMatt already nit it, hehe.
I'm fine with doing this separately, untested ack (nothing to test, really).
I'll put more attention into these details the next time.
Maybe this is the right place for a clang commit on the new files?

@sipa
Copy link
Member

sipa commented Sep 9, 2014

Can we change the license of existing code?

@laanwj
Copy link
Member

laanwj commented Sep 9, 2014

As discussed before we're not changing the license, just clarifying it.

@Diapolo
Copy link
Author

Diapolo commented Sep 9, 2014

@sipa Dunno, @TheBlueMatt suggested this and I just included it, because this is a seperate folder anyway (script/).

@laanwj
Copy link
Member

laanwj commented Sep 9, 2014

ACK anyhow.

@sipa
Copy link
Member

sipa commented Sep 9, 2014

I thought we weren't sure about legality of changing the license (even just the name) of existing code, but were going to use MIT (without X11) in new code only. I don't care either way - just want to make sure we know what we're doing.

@jtimon
Copy link
Contributor

jtimon commented Sep 9, 2014

The latest license discussion was in #4832.
What I do by default is copying the license from another file (and I suspect most people do this same thing), so whatever license it is, we may want to merge something like #4832 once instead of having this discussion every time someone creates a new file just copying the license. I think I would do the same with little corrections like the missing comment at the end of the #endif of the .h file.
These mistakes are much harder to be repeated if people use existing files as templates and we already have what we want in existing files. Although a PR fixing all licenses and endif comments will likely touch many files, it will probably merge cleanly with any other changes in the same files.
And although a PR fixing all alphabetic orderings is more likely to produce conflicts you may want to do it once and police that nobody breaks it from then on as well. At least I wouldn't oppose to it.
I know these things aren't high priority, I'm just expressing my preference to fix these things at once over fixing it little by little first in new files and then in many small PRs or included in other PRs that are touching those parts of the code anyway like with clang formatting.

@sipa
Copy link
Member

sipa commented Sep 9, 2014

ACK

@Diapolo
Copy link
Author

Diapolo commented Sep 14, 2014

Anything left that needs to be done here to help speed up merge?
I rebased to catch the new wallet_ismine changes.

- add missing header end comments
- ensure alphabetical ordering
- update copyright year and license
@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4881_2d79bba36b028dd803fb17124420b9d209b842b6/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@jtimon
Copy link
Contributor

jtimon commented Sep 14, 2014

I don't think so, it looks good to me as it is.

@sipa sipa merged commit 2d79bba into bitcoin:master Sep 14, 2014
sipa added a commit that referenced this pull request Sep 14, 2014
2d79bba cleanup new script files (no code changes) (Philip Kaufmann)
@Diapolo Diapolo deleted the cleanup_script branch September 15, 2014 05:58
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants