----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://fedorahosted.org/reviewboard/r/304/#review481 -----------------------------------------------------------

src/rolekit/server/io/rolesettings.py https://fedorahosted.org/reviewboard/r/304/#comment1017

Given that this is now a full path instead of just a filename, it would probably be a good idea to rename this to self.filepath

src/rolekit/server/io/rolesettings.py https://fedorahosted.org/reviewboard/r/304/#comment1018

Checking for os.path.exists() here is redundant and faulty. You should just do try: os.remove(self.filename) except OSError: pass

The original method contained a race-condition where if the file was removed by another process between the exists() and the remove(), we'd have an unhandled exception.

- Stephen Gallagher

On July 10, 2014, 12:08 p.m., Thomas Woerner wrote:

---

This is an automatically generated e-mail. To reply, visit: https://fedorahosted.org/reviewboard/r/304/

---

(Updated July 10, 2014, 12:08 p.m.)

Review request for rolekit reviewers, Stephen Gallagher and Thomas Woerner.

Repository: rolekit

Description

Simplified src/rolekit/server/io/rolesettings.RoleSettings, new remove method

Diffs

src/rolekit/server/io/rolesettings.py 671a40a36be363a4d0e8f9446dc4e1c2277cbcd5

Diff: https://fedorahosted.org/reviewboard/r/304/diff/

Testing

Thanks,

Thomas Woerner