Resolved a to do by changing the year of the copyright to be dynamic #91
Conversation
| @@ -19,7 +19,7 @@ | |||
| from superflore.utils import resolve_dep | |||
| from superflore.utils import sanitize_string | |||
| from superflore.utils import trim_string | |||
|
|
|||
| from time import gmtime, strftime | |||
There was a problem hiding this comment.
This should be above in the block with the other system imports (just below sys) And leave the white space here.
| """ | ||
| ret = "# Copyright 2017 " + distributor + "\n" | ||
| ret = "# Copyright " + strftime("%Y", gmtime()) + distributor + "\n" |
There was a problem hiding this comment.
You're missing whitespace after the year.
- # Copyright 2017Open Source Robotics Foundation
+ # Copyright 2017 Open Source Robotics Foundation
There was a problem hiding this comment.
This was caught by the testing but it shows that next year it will be a problem for the testing. The test will need to be extended with similar logic. It's hard coded to 2017 here: https://github.com/ros-infrastructure/superflore/blob/master/tests/ebuild/simple_expected.ebuild#L1
There was a problem hiding this comment.
Thank you so much for this feedback, I will get on changing these problems ASAP! :)
|
Seems there's an unresolved merge conflict. @shaneallcroft It would also be good to update the test to be dynamic as well, since the year is hard coded (though that could be a separate PR, imo). |
| @@ -92,9 +98,8 @@ def get_ebuild_text(self, distributor, license_text): | |||
| Generate the ebuild in text, given the distributor line | |||
| and the license text. | |||
|
|
|||
There was a problem hiding this comment.
Please remove this blank line as well -- was there only to put a space between the comment and the TODO.
|
@tfoote Do you think the test case should be a separate PR, or should it be addressed here? |
|
It would be good to resolve here, otherwise it's a ticking time bomb, that will block development in the future. |
|
Gotcha, ty, I'll get on changing that tn :) |
|
Sorry for the delay, there was a school project that took alot longer than expected last night. Is test_simple the only test i need to touch in this case? No other tests I saw touched an ebuild file. |
|
wait damn it failed lemme check that out |
|
You will probably need to change the example ebuild output to be templated on the year when loaded. |
|
Hmmmmm, I thought that's what I had just done, it's saying its failing because the text obtained from the file is empty, I may have just made a weird mistake. I might be able to fix it super easily. Gonna try now. |
|
Also yes! :) looks like my change worked |
tests/test_ebuild.py
Outdated
| expect_file.read(16) | ||
| fileUpdateStr = "# Copyright " + strftime("%Y", gmtime()) | ||
| fileUpdateStr += expect_file.read() | ||
| expect_file.write(fileUpdateStr) |
There was a problem hiding this comment.
You don't really want to write back to the file here.
I'd suggest loading the file and then using the python regex library sub function on the read data. https://docs.python.org/2/library/re.html#re.sub
it should be something like:
correct_text = re.sub('Copyright 2017', 'Copyright " + strftime("%Y", gmtime(), correct_text)
There was a problem hiding this comment.
Ahhh, gotcha, ty, I'll get on that
| @@ -0,0 +1,24 @@ | |||
| # Copyright 2017 Open Source Robotics Foundation | |||
There was a problem hiding this comment.
This file looks to have been committed added by mistake?
|
Thanks this looks good now execpt the extra file |
|
Sweet! & I'm on it |
|
Holy darn, I didn't even realize at first that the file in the PR changes was there just because it was a copy of simple_expected by another name, I thought the problem was the repo was thinking I made changes to simple_expected when I actually hadn't. Totally my B, I definitely should've realized that sooner haha, I think i may need some sleep soon 👍 the extraneous file mustve been created while I was testing the version of the ebuild test case that wrote to the file maybe? And then I accidentally git added it? Regardless, I think everything is good to go now :) @tfoote and @allenh1 let me know if there's any more changes you guys want me to make for this PR before its merged. Thank you so much for the guidance on this!!! |
|
The extra file's clear, but you rolled back the regex change too. It should be restored. |
|
We should both get some sleep. |
|
Oh my gosh, how the heck did I even manage that??! ty for catching it. And haha yeah we should definitely get some sleep. Thank you so much again for all your help. Have a goodnight! 👍 :) |
…os-infrastructure#91) * Resolved a todo by changing the year of the copyright to be dynamic * Update test_ebuild.py to also be dynamic
Before this pull request, the copy right year was unchanging. With the code in this pull request implemented however, the year will change automatically as years change in real life.