Added feature to write output to log file (write stdout to log file #66) #106

roykrikke · 2021-04-02T20:22:10Z

Hello Ivan,

I added a news feature. Added feature to write output to log file, it is linked to feature request 66

Greetings,

Roy

ivandokov

Please add tests for this feature

phockup.py

roykrikke · 2021-04-04T20:24:45Z

I will implement some tests and update the TODO.

Open question: Is there a reason that you did not used https://docs.python.org/3.7/howto/logging.html? I feel it's easy to separate logging, debugging and operational views a bit nicer? But you may have a good reason not to. Therefor my open question out of curiosity.

ivandokov · 2021-04-05T07:48:28Z

Yes I have a reason - this is the only project I ever coded in Python and I'm not familiar with all the nice tools that the language provides. Essentially I didn't know about this so you are more than welcome to implement any language provided niceties as I prefer proven libraries over DIY solutions.

…s now add a higher percentage. Added tests for feature "Limit directory traversal depth" because this was missing. Made the code PEP 8 compliant. Updated the readme: - Added icon for MIT license - Added icon for PEP8 check - Added chapter in Development section

roykrikke · 2021-04-20T10:58:31Z

Added tests for feature "Limit directory traversal depth" because this was missing.
Made the code PEP 8 compliant.
Updated the readme:

Added icon for MIT license
Added icon for PEP8 check
Added chapter in Development section
Update source code to use https://docs.python.org/3.7/howto/logging.html library (including feature of using --debug / --quiet / --log-file)

I hope you appreciate my work. I wrote it with the intention of making phockup even more attractive to many more users.

While I've been working on these changes I've gotten even more ideas. I will wait and see what you think about this, then we can discuss the other ideas.

ivandokov · 2021-04-20T11:44:29Z

Wow outstanding work! It will take me some time to review all these changes but I very much appreciate what you've done for the project!

roykrikke · 2021-04-20T14:38:46Z

Nice, let me know if you have any questions. When all is fine and merge I will contact you about some other ideas.

unapproachable · 2021-04-24T17:23:48Z

Regarding the formatting changes, is this the result of being blackd? If so, maybe we can get a PR to get that check to run as one of the github workflow checks to prevent all the whitespace and line-wrapping changing for future PRers (like me).

Secondly, is there any appetite for using colored logs or something like it? My main use case would be for duplicates. In a wall of console output, it's nice to be able to scroll and see the files that didn't transfer for whatever reason. Not trying to pile on for this PR, but figured this was as good a place as any to discuss while the writer is being modernized.

Lastly, as I'm also new to Python, is there a reason for going with .format() instead of f-string? I feel the code reads much better with the variables in place rather than tupled at the end of the log message.

roykrikke · 2021-04-24T18:34:07Z

Regarding the formatting changes, is this the result of being blackd? If so, maybe we can get a PR to get that check to run as one of the github workflow checks to prevent all the whitespace and line-wrapping changing for future PRers (like me).

Secondly, is there any appetite for using colored logs or something like it? My main use case would be for duplicates. In a wall of console output, it's nice to be able to scroll and see the files that didn't transfer for whatever reason. Not trying to pile on for this PR, but figured this was as good a place as any to discuss while the writer is being modernized.

Lastly, as I'm also new to Python, is there a reason for going with .format() instead of f-string? I feel the code reads much better with the variables in place rather than tupled at the end of the log message.

What exactly do you mean by "being blackd"?
PEP8 checks maybe could checked with one of the github workflow checks. I hadn't thought of including it in github yet.

About you idea on colored duplicates is a good one, but the https://pypi.org/project/coloredlogs/ is only putting colors to the levels of logging. In addition, you can only see the colors in the live console output and not in the log file that you can also create.

Last but not least, I reformatted the code from % to .format() because I thought the latter was the correct method. I was not aware of f-string. After reading https://realpython.com/python-f-strings/ I think I have to reformat the code to f-strings.

I'll wait to see what ivandokov thinks of all my additions and adjustments. If it's oke for him, I suggest that we use f-strings for the merge in the code.

unapproachable · 2021-04-25T12:39:40Z

"being blackd" refers to post-processing code before commits using the black formatter (IDEs connect via the daemon, blackd).

https://github.com/psf/black#blackd

It's a standardized way to format code with PEP8 in mind. It has helped keep code between contributors standardized and removes the variance in code-style automatically. Github supports it as one of its workflow checks, thus ensuring all committers have compliant code.

I don't know of a way to get ANSI (or otherwise) colors into console logs in Python, so this may be a PR for a later date. Colors not appearing in log files isn't a concern for me, as the messaging allows me to mark and delete the noise. The idea was more specifically related to interactive use at the console, possibly using logger.warning or logger.error to achieve colorized output.

Again, I'm new to Python, so please verify/validate my statements. These ideas can both live on in separate PRs and thank you for the work on getting the logging updated to allow for future flexibility.

roykrikke · 2021-05-02T12:02:10Z

I don't want to push, I just ask out of curiosity. When do you have time to look at my code changes?

ivandokov · 2021-05-02T12:03:42Z

I'm sorry for the delay I had a busy week. It's Easter today so I'll be available probably tomorrow.

roykrikke · 2021-05-02T12:05:55Z

I'm sorry for the delay I had a busy week. It's Easter today so I'll be available probably tomorrow.

NP, take you time.

ivandokov

To be honest the formatting change in this PR is too massive and adds too much noice to the introduced features. Also some of the formatting changes are actually counter-productive to me and I commented few of them in the PR.

Regarding the strings formatting I was only aware of the f-string functionality and you made me do a little research and according to this article there is a newer cooler way to format strings with formatted string literals - https://realpython.com/python-f-strings/
We support python 3.6+ so its fine to use these.

ivandokov · 2021-05-02T19:28:34Z

phockup.py

@@ -116,27 +134,33 @@ def main():
        default=-1,
        choices=range(0, 255),
        metavar="1-255",
-        help="Descend at most 'maxdepth' levels (a non-negative integer) of directories",
+        help="""\


I don't get this formatting here. I understand the line wrapping of the multiline comments but this one can be on the same line. Is this a PEP8 requirement?

It's not a PEP8 requirement for PEP8 to do it like this as long it's within 79 characters per line. I thought everywhere in the code to keep the same formatting.

ivandokov · 2021-05-02T19:29:12Z

phockup.py

+        action="store_true",
+        default=False,
+        help="""\
+Enable debugging.


Same here. I don't like this taking 3 lines instead of 1. Please let me know if this is PEP8 requirement.

It's not a PEP8 requirement for PEP8 to do it like this as long it's within 79 characters per line. I thought everywhere in the code to keep the same formatting.

ivandokov · 2021-05-02T19:30:47Z

src/date.py

@@ -44,7 +47,8 @@ def from_exif(self, exif, timestamp=None, user_regex=None, date_field=None):
        # sometimes exif data can return all zeros
        # check to see if valid date first
        # sometimes this returns an int
-        if datestr and isinstance(datestr, str) and not datestr.startswith('0000'):
+        if datestr and isinstance(datestr, str) and not \


Can we keep this on one line?

To keep it in one line is crossing the 79 characters per line requirement from PEP8.

ivandokov · 2021-05-02T19:37:36Z

tests/test_date.py

@@ -101,8 +101,9 @@ def test_get_date_custom_regex():
    """
    A valid regex with a matching filename. Returns a datetime.
    """
-    date_regex = re.compile(r"(?P<day>\d{2})\.(?P<month>\d{2})\.(?P<year>\d{4})[_-]?(?P<hour>\d{2})\.(?P<minute>\d{2})\.(?P<second>\d{2})")
-    assert Date("IMG_27.01.2015-19.20.00.jpg").from_exif({}, False, date_regex) == {
+    date_regex = re.compile(r"(?P<day>\d{2})\.(?P<month>\d{2})\.(?P<year>\d{4})[_-]?(?P<hour>\d{2})\.(?P<minute>\d{2})\.(?P<second>\d{2})")     # noqa: E501


What's #noqa: E501

If code is checked for PEP8 you can exclude a rule, in this case rule E501. PEP8 lint tools will skip a line with e.g. #noqa: E501.
In this case I made a decision that splitting this line would be counter-productive. To keep it in one line is crossing the 79 characters per line requirement from PEP8 therefor the #noqa: E501.

ivandokov · 2021-05-02T19:42:19Z

I think having an automated way such as Github Actions to format the code is going to be awesome. The PEP8 standard looks like the the way to go even though 79 characters line limit is super short for me.

ivandokov · 2021-05-02T19:43:54Z

Oh I forgot to comment the idea for colored output - I LOVE IT!

roykrikke · 2021-05-02T21:06:26Z

ink having an automated way such as Github Actions to format the code is going to be awesome. The PEP8 standard looks like the the way to go even though 79 characters line limit is super short for me.

I agree with that with the automated PEP8 checks and also with the line limit of 79 characters. Perhaps we can have a general rule to make the larger max-line-length.

I was looking at tools in github that would help with this. To this end, I found a tool called pep8speaks (https://github.com/OrkoHunter/pep8speaks) which is a github integration. After every pull request is created, the integration scans through any python code as part of the commits, and runs a pep8 checker on it. After checking, a comment if posted to the pull request with all the PEP8 errors or warnings present.

You can see an example here.
m-anish#2 (comment)

Should we try integrating it within phockup?

PS I'm not the owner of the repo, so I need help from you ivandokov.

roykrikke · 2021-05-02T21:06:55Z

Oh I forgot to comment the idea for colored output - I LOVE IT!

I also agree, but baby steps :-)

roykrikke · 2021-05-02T21:20:24Z

To be honest the formatting change in this PR is too massive and adds too much noice to the introduced features. Also some of the formatting changes are actually counter-productive to me and I commented few of them in the PR.

Regarding the strings formatting I was only aware of the f-string functionality and you made me do a little research and according to this article there is a newer cooler way to format strings with formatted string literals - https://realpython.com/python-f-strings/
We support python 3.6+ so its fine to use these.

I understand it is overwhelming. That is a lesson learned for me, baby steps next time ;-). As described above I can imagine that you feel some changes are counter-productive. Sorry for that, was not my intention!

As I already responded to a message from "unapproachable", I reformatted the code from % to .format() because I thought the latter was the correct method. I was not aware of f-string either . After reading https://realpython.com/python-f-strings/ I think I/we have to reformat the code to f-strings.

I also wrote above to "unapproachable": (I'll wait to see what ivandokov thinks of all my additions and adjustments. If it's oke for him, I suggest that we use f-strings for the merge in the code.)

Which brings me to the following questions:

What do you think of the functionality I have added?
If you like the functionality, what should the next steps be in your view to merge the functionality and also add code formatting?

ivandokov · 2021-05-02T21:52:03Z

I don't like some of the formatting changes but I don't want to block the PR for that (even though I should).
I'm merging it and we can clean it up with the automated code style checker PR.

roykrikke · 2021-05-04T05:52:57Z

I don't like some of the formatting changes but I don't want to block the PR for that (even though I should).
I'm merging it and we can clean it up with the automated code style checker PR.

Hello ivandokov,

I will have a look at the code to change it to python-f-strings as a first step. As a second step I will see if I can integrate pep8speaks.

Both in a separated PR. For pep8speaks I expect that we have to work together in a slightly different way. But I'm going to see if I can do the prep work in a forked repo as a starting point.

I hope, I get to the first step done this week.

tianleiwu · 2021-05-30T09:37:50Z

phockup.py

+    )
+
+    parser.add_argument(
+        "--log-file",


Change to "--log", otherwise options.log in line 240 will throw exception.

@roykrikke can you please take a look at this and if there is missing test not catching this issue to add one. Of course if you feel like doing it.

@ivandokov and @tianleiwu I will have a look add the issue, no problem. I will also check if there is a missing test case. But to be honest, at the highest level we are missing test cases anyway. Idea: let's work together with a few people to realize high-level test cases. Who wants to help with this?

I did change the --log-file to --log as I had to fix something else and I couldn't run phockup without changing this. It didn't broke any tests so I guess I haven't break anything but it's worth looking at.

@ivandokov and @tianleiwu I cannot reproduce the reported error.

In the current master branch all is fine.
if options.log: logfile = os.path.expanduser(options.log) fh = logging.FileHandler(logfile) fh.setFormatter(formatter) logger.addHandler(fh)

did change the --log-file to --log as I had to fix something else and I couldn't run phockup without changing this. It didn't broke any tests so I guess I haven't break anything but it's worth looking at.

Ha oke, now i get it again. Strange that your comment was not visible yesterday when I wrote the above. Oh well... issue solved lets continue...

roy added 4 commits March 28, 2021 19:13

Added info about two flags (quiet/dry-run)

126fab7

Added missing word.

5b3acf2

Resolved merge conflict

4d7476a

Added feate to wtite output to log file.

6a767e9

ivandokov reviewed Apr 3, 2021

View reviewed changes

phockup.py Outdated Show resolved Hide resolved

roy added 5 commits April 18, 2021 19:21

Removed printer class, change print output and logging via logging.

c11a543

Added tests and made code compatible with flake8.

679a188

Small update to readme.

bd5f65e

I forgot to add the missing new files in the tests/input folder.

a1d8976

ivandokov reviewed May 2, 2021

View reviewed changes

ivandokov merged commit c0fc9fa into ivandokov:master May 2, 2021

tianleiwu reviewed May 30, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added feature to write output to log file (write stdout to log file #66) #106

Added feature to write output to log file (write stdout to log file #66) #106

roykrikke commented Apr 2, 2021

ivandokov left a comment

roykrikke commented Apr 4, 2021

ivandokov commented Apr 5, 2021

roykrikke commented Apr 20, 2021

ivandokov commented Apr 20, 2021

roykrikke commented Apr 20, 2021

unapproachable commented Apr 24, 2021

roykrikke commented Apr 24, 2021

unapproachable commented Apr 25, 2021

roykrikke commented May 2, 2021

ivandokov commented May 2, 2021

roykrikke commented May 2, 2021

ivandokov left a comment

ivandokov May 2, 2021

roykrikke May 2, 2021

ivandokov May 2, 2021

roykrikke May 2, 2021

ivandokov May 2, 2021

roykrikke May 2, 2021

ivandokov May 2, 2021

roykrikke May 2, 2021

ivandokov commented May 2, 2021

ivandokov commented May 2, 2021

roykrikke commented May 2, 2021 •

edited

Loading

roykrikke commented May 2, 2021

roykrikke commented May 2, 2021

ivandokov commented May 2, 2021 •

edited

Loading

roykrikke commented May 4, 2021

tianleiwu May 30, 2021

ivandokov Jun 2, 2021

roykrikke Jun 2, 2021

ivandokov Jun 6, 2021 •

edited

Loading

roykrikke Jun 6, 2021 •

edited

Loading

roykrikke Jun 7, 2021 •

edited by ivandokov

Loading

Added feature to write output to log file (write stdout to log file #66) #106

Added feature to write output to log file (write stdout to log file #66) #106

Conversation

roykrikke commented Apr 2, 2021

ivandokov left a comment

Choose a reason for hiding this comment

roykrikke commented Apr 4, 2021

ivandokov commented Apr 5, 2021

roykrikke commented Apr 20, 2021

ivandokov commented Apr 20, 2021

roykrikke commented Apr 20, 2021

unapproachable commented Apr 24, 2021

roykrikke commented Apr 24, 2021

unapproachable commented Apr 25, 2021

roykrikke commented May 2, 2021

ivandokov commented May 2, 2021

roykrikke commented May 2, 2021

ivandokov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivandokov commented May 2, 2021

ivandokov commented May 2, 2021

roykrikke commented May 2, 2021 • edited Loading

roykrikke commented May 2, 2021

roykrikke commented May 2, 2021

ivandokov commented May 2, 2021 • edited Loading

roykrikke commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivandokov Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

roykrikke Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

roykrikke Jun 7, 2021 • edited by ivandokov Loading

Choose a reason for hiding this comment

roykrikke commented May 2, 2021 •

edited

Loading

ivandokov commented May 2, 2021 •

edited

Loading

ivandokov Jun 6, 2021 •

edited

Loading

roykrikke Jun 6, 2021 •

edited

Loading

roykrikke Jun 7, 2021 •

edited by ivandokov

Loading