2
\$\begingroup\$

I have been working with where I use watchdogs to be able read whenever I have created a file by the name -> kill_.flag and it contains:

*kill_*.flag contains:

hello;https://www.google.se/;123

and my script is then going to read the created file, split the ";" between each other and give each a key where I later on print it out which I for now has the text "Deleting" where I will continue my script after I have completed this part.

# !/usr/bin/python3
# -*- coding: utf-8 -*-

import os
import os.path
import time

import watchdog.events
import watchdog.observers


class AutomaticCreate(watchdog.events.PatternMatchingEventHandler):

    def __init__(self):
        self.delete_flag = "*kill_*.flag"
        watchdog.events.PatternMatchingEventHandler.__init__(self, patterns=[os.path.split(self.delete_flag)[1]],
                                                             case_sensitive=False)

    def on_created(self, event):
        while True:
            try:
                with open(event.src_path) as f:
                    content = f.read().split(";")
                    payload = {
                        "name": content[0].strip(),
                        "link": content[1].strip(),
                        "id": content[2].strip()
                    }
                break
            except OSError:
                print(f"Waiting for file to transfer ->  {event.src_path}")
                time.sleep(1)
                continue

        while True:
            try:
                os.remove(event.src_path)
                break
            except Exception as err:
                print(f"Trying to remove file -> {err}")
                time.sleep(1)
                continue

        print(f'Deleting: {payload["name"]} - {payload["link"]} - {payload["id"]}')

    def main(self):
        observer = watchdog.observers.Observer()
        observer.schedule(AutomaticCreate(), path=os.path.split(self.delete_flag)[0], recursive=True)
        observer.start()
        try:
            while True:
                time.sleep(1)
        except KeyboardInterrupt:
            observer.stop()
        observer.join()


if __name__ == "__main__":
    AutomaticCreate().main()

I wonder if there is a smarter or maybe even cleaner way to improve this code both performance and whatever it can be?

\$\endgroup\$
4
  • \$\begingroup\$ Are you using Python 2? \$\endgroup\$
    – Reinderien
    Mar 18, 2021 at 13:05
  • \$\begingroup\$ Python 3 :) @Reinderien \$\endgroup\$ Mar 18, 2021 at 13:15
  • 2
    \$\begingroup\$ Ok. Please file a new question with your updated code. In the meantime I'll need to roll back your edit to this one. \$\endgroup\$
    – Reinderien
    Mar 18, 2021 at 13:16
  • \$\begingroup\$ I have now created a new question in here: codereview.stackexchange.com/questions/257341/… :) @Reinderien \$\endgroup\$ Mar 18, 2021 at 13:24

1 Answer 1

4
\$\begingroup\$

I genuinely hope you don't take offence by this, but this implementation is so insane that it's frankly impressive.

Since you have delete_flag = "*kill_*.flag", calling os.path.split on it is pointless. Perhaps you're substituting that with something else that has a path, but if you are, why isn't it parametrized?

Do a tuple-unpack here:

                    content = f.read().split(";")
                    payload = {
                        "name": content[0].strip(),
                        "link": content[1].strip(),
                        "id": content[2].strip()
                    }

should just be

name, link, id = (part.strip() for part in f.read().split(';'))

Putting these in a payload dict is actively harmful. You only ever use these as variables later. About the most generous I can be here is to guess that there's actually more code in your application that you haven't shown which relies on the payload for some other network or serialized file operation, but if that's the case the whole question is invalid.

AutomaticCreate implements PatternMatchingEventHandler - so then why are you reinstantiating it here?

        observer.schedule(AutomaticCreate()

You already instantiated it here:

AutomaticCreate().main()

Keep the second instantiation, and replace the first instantiation with a reference to self.

The following is a strange and non-standard way of calling a base constructor:

    watchdog.events.PatternMatchingEventHandler.__init__(self, patterns=[os.path.split(self.delete_flag)[1]],  # ...

Just use super().

Rework your class to be a context manager, where __enter__ calls self.observer.start(), and __exit__ calls self.observer.stop(). Then, use your class instance in a with at the top level and delete your main() method.

\$\endgroup\$
3
  • \$\begingroup\$ name, link, id = (part.strip() for part in f.read().split(';')) I think I more elegant way would be name, link, id = map(str.strip, f.read().split(';')) \$\endgroup\$
    – warvariuc
    Mar 18, 2021 at 6:01
  • \$\begingroup\$ elegant but harder to read ;) \$\endgroup\$ Mar 18, 2021 at 6:59
  • \$\begingroup\$ I have now created a new (Well updated the thread) with the new code with your guidens! Could you please look at it again and tell me if there is anything im missing? :) \$\endgroup\$ Mar 18, 2021 at 8:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.