search

hartwork / mozilla-password-decrypt

:unlock: Decrypt passwords stored by Firefox, Thunderbird, Iceweasel, Icedove using libnss3.so
7 stars 4 forks source link

Some changes #4

Closed nyov closed 9 years ago

nyov commented 9 years ago

Also rewrote this to use a namedtuple for logins/sites, generalized the autodetected profile paths to include non-default profiles (, and to what I think is a slightly better format according to pep8 for the imports)

hartwork commented 9 years ago

Hi!

While I'm happy to see a new pull request, I don't like the fact that a lot of unrelated changes all make a single commit. To give emaples, besides pysqlite2 support I see:

Please turn that into commits with distinct semantics each and use make targets "isort" and "pep8". Okay?

nyov commented 9 years ago

I suppose I can split them. What is isort?

hartwork commented 9 years ago

What is isort?

isort is https://github.com/timothycrosley/isort pip install isort or so might work.

nyov commented 9 years ago

I would disagree with enforcing isort. It's okay as a reference.

And forcing -m 2 is counter to pep8's recommendation for grouping.

The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation.

If you believe in forcing this, option 3 would make more sense for the least amount of unrelated LOC changes and resorting in future commits?

hartwork commented 9 years ago

I would disagree with enforcing isort. It's okay as a reference.

The pull request currently re-formats existing import lines. If it does so, it should be a distinct commit in any case. For new import lines, I would not request a specific import line style strictly (i.e. forcing). I would run isort some time myself later though, probably.

If you believe in forcing this, option 3 would make more sense for the least amount of unrelated LOC changes and resorting in future commits?

I have switched to -m 4 in commit 6ba77a90a0b000f7601a67c098cc3cadc251b25e now. Are you okay with -m 4?

nyov commented 9 years ago

Sure, let's not get too caught up in semantics. Let me know if the split commits are okay now.

hartwork commented 9 years ago

I have cheery-picked these commits:

That saved the merge commit (that would pull in pre-review commits) and commits with mixed semantics.

hartwork commented 9 years ago

Main changes cherry-picked, closing.