License maintainer #40
Conversation
…r license in order and fails if not. Imported from https://github.com/NitorCreations/RobotFramework-EclipseIDE
…e not touched by files currently staged for commit Imported from https://github.com/NitorCreations/CoreComponents
Conflicts: .githooks/LICENSE .githooks/README.md
…utes instead. The "licensefile" attribute should now be set for files and patterns to choose what license template to use.
Formatting
Fix broken LICENSE-* paths
Change the author regexp to allow author "names" to end with a ">" character thus permitting such names to include authors' email address marked in the common way of surrounding it with pointy brackets.
Allow author names to contain email addresses
…cense-maintainer. Merge remote-tracking branch 'githooks-license-maintainer/hooks-only' into develop
There was a problem hiding this comment.
I think that since this PR changes so many files and is specifically for the license maintainer, then it shouldn't include completely unrelated changes too that may go unnoticed amongst the mass of comments. I don't have any issues with the license maintainer changes, but I think the changes to the unit/integration tests should be in a separate PR that solves the problem of running all the test files when we run cargo test (including those in the tests/ folder when we re-introduce it)
I agree that the 3x tests that are currently in src/main.rs would be more suitable as integration tests in the tests/cli_test.rs, but there's no point moving them there if they are just going to be ignored with the #[ignore] macro and when running cargo test doesn't even pick up files in that folder, otherwise we won't know if we're breaking the codebase when we're making changes, and especially if a duplicate copy of each of them is going to be left in the src/main.rs file where we know they are actually being picked up when you run cargo test.
I noticed that in this PR also includes a change whereby the 3x tests that are in src/main.rs and are currently passing have not only been left in src/main.rs, but also now been added back into tests/cli_test.rs with the #[ignore] macro, so we have a duplicate copy of all 3 of them, but only the ones in src/main.rs run when you run cargo test. As we agreed previously, it's not satisfactory for us to have to run all the tests by running cargo test --test cli_test && cargo test, when simply running cargo test should run all the tests. So since running cargo test still isn't picking up the tests in tests/cli_test.rs at all yet, there's no point re-creating that tests/ folder and moving those 3x tests back in there until we solve the underlying problem of actually being able to run all the tests with the single command cargo test including the ones in tests/cli_test.rs, and having them pass the tests.
So that's my feedback. If we merge the proposed changes then we should have to create an "Issue" that requires us to figure out how to run all the tests including those in the tests/ folder with just cargo test, and when we figure that out we'll need to unignore the tests in the tests/ folder and ensure that they actually run, and when we have that assurance we'll proceed to remove the duplicates tests from src/main.rs. I'm happy to change my review from "Changes requested" to "Approved" if we agree to the requirement to create the "Issue", and if we don't care about the unit tests changes having been included in this PR and think it's unnecessary to have them included in a separate PR.
What does @ChosunOne think?
|
I think those changes must have been included by mistake, I'll fix that. I left out @ChosunOne's last name deliberately because I think he prefers to retain a level of anonymity. |
|
I wish you would talk to me before going and making changes, there were reasons why I had the first line like "AUTHORS, James Ray, Josiah @ChosunOne, and Luke Schoen". When I was making these changes, the AUTHORS which occurs twice in the license header was being replaced. However the author of the repo recognized that was an issue and may have fixed that since then. |
|
I will wait for a confirmation from @ChosunOne for his preference about his name and the license header before making these changes in another PR with just the changes for the license maintainer, or update this PR accordingly. |
|
I'm going to assume that @ChosunOne prefers not to add his last name, if he prefers otherwise he can change that later. There have been no changes to the license-maintainer repo, so we will need to use the format that I had originally, at least until the repo is fixed. NitorCreations/license-maintainer#9 (comment) |
|
I'm going to assume that @ChosunOne prefers not to add his last name, if he prefers otherwise he can change that later. There have been no changes to the license-maintainer repo, so we will need to use the format that I had originally, at least until the repo is fixed. |
|
I'm making another PR, but am just trying to make hacky fixes to get it to "work" again. |
|
I'm making modifications to https://github.com/NitorCreations/license-maintainer without references to year or authors as they aren't needed for the Unlicense. Further updates will be posted at NitorCreations/license-maintainer#9 and in a new PR. |
jamesray1
added
P7: nice to have
|
See openethereum/parity-ethereum#8666 for inspiration. |
This updates the license header for previous commits to match a template, as well as checking future commits to make sure that the license header matches with the template, but there are merge conflicts to fix. However, it doesn't work properly.
Alternatively, with research an easier alternative could possibly be found.