borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
15
57
Fork 40
Code Issues 53 Pull Requests 2 Actions Packages Releases 121 Activity

Integrate isort #173

Merged
witten merged 2 commits from :cifix/isort-integration into master 2019-05-14 17:02:37 +00:00
Conversation 5 Commits 2 Files Changed 37 +56 -54
decentral1se commented 2019-05-13 21:40:17 +00:00
Contributor
Copy Link

Closes witten/borgmatic#169.

Isort now agrees with Black. But do you!? :)

See https://isort.readthedocs.io/en/latest/.

Closes https://projects.torsion.org/witten/borgmatic/issues/169. Isort now agrees with Black. But do you!? :) See https://isort.readthedocs.io/en/latest/.
witten commented 2019-05-13 21:48:50 +00:00
Owner
Copy Link

Man, you're on đŸ”„ with regards to the PRs! Trying to keep up. 😃

In regards to the import diffs in this changeset: I welcome our new robot overlords.

Question though.. What do you think of adding an invocation of isort --check-only within the main tox environment? That way, much like with black, invalidly ordered imports will break the build and require attention.

Man, you're on :fire: with regards to the PRs! Trying to keep up. :smiley: In regards to the import diffs in this changeset: I welcome our new robot overlords. Question though.. What do you think of adding an invocation of `isort --check-only` within the main tox environment? That way, much like with black, invalidly ordered imports will break the build and require attention.
decentral1se commented 2019-05-13 21:54:53 +00:00
Author
Contributor
Copy Link

Lol! I've revised the patch to run isort in the main Tox environment and also try to follow more closely the full --* option style and script usage. See what you reckon.

Lol! I've revised the patch to run `isort` in the main Tox environment and also try to follow more closely the full `--*` option style and script usage. See what you reckon.
witten reviewed 2019-05-13 23:27:49 +00:00
tox.ini
@ -25,0 +26,4 @@
[testenv:isort]
deps = {[testenv]deps}
commands =
isort {posargs:--recursive --check-only} --settings-path setup.cfg .
witten commented 2019-05-13 23:27:18 +00:00
Owner
Copy Link

Should --check-only be removed here? I thought that the idea is, much like with the testenv:black tox environment, this testenv:isort environment is for actually doing the re-sorting for you. I may be misunderstanding the intent though.

Should `--check-only` be removed here? I thought that the idea is, much like with the `testenv:black` tox environment, this `testenv:isort` environment is for actually doing the re-sorting for you. I may be misunderstanding the intent though.
decentral1se commented 2019-05-14 11:19:03 +00:00
Author
Contributor
Copy Link

Ah yes, that's a good idea to have the same interface. Will fix.

Ah yes, that's a good idea to have the same interface. Will fix.
witten reviewed 2019-05-13 23:28:56 +00:00
witten left a comment
Owner
Copy Link

Thanks.

Thanks.
witten reviewed 2019-05-13 23:30:06 +00:00
scripts/isort
@ -0,0 +4,4 @@
isort --recursive --check-only --settings-path setup.cfg .
else
echo "Skipping isort due to not being installed."
fi
witten commented 2019-05-13 23:30:01 +00:00
Owner
Copy Link

I don’t think you have to do the “is installed” check for isort, since you’re unconditionally installing it as part of test requirements, right? The only reason we do the presence check for black is because it doesn’t work on certain versions of Python (<3.6), and I didn’t want to prevent borgmatic tests from running for Python 3.5 (as long as that version is still supported).

I don’t think you have to do the “is installed” check for isort, since you’re unconditionally installing it as part of test requirements, right? The only reason we do the presence check for black is because it doesn’t work on certain versions of Python (<3.6), and I didn’t want to prevent borgmatic tests from running for Python 3.5 (as long as that version is still supported).
decentral1se commented 2019-05-14 11:22:20 +00:00
Author
Contributor
Copy Link

Ah yes, that makes sense. Will fix.

Btw, if you move the logic of "run which thing on which Python" into Tox, you can remove the need for these scripts. Then, you can do stuff like:

commands = 
    pytest
    py36: black .
    flake8 .

I am a big fan of Tox ;)

Ah yes, that makes sense. Will fix. Btw, if you move the logic of "run which thing on which Python" into Tox, you can remove the need for these scripts. Then, you can do stuff like: ``` commands = pytest py36: black . flake8 . ``` I am a big fan of Tox ;)
decentral1se commented 2019-05-14 11:24:29 +00:00
Author
Contributor
Copy Link

Comments addressed 👍

Comments addressed :thumbsup:
witten commented 2019-05-14 16:54:11 +00:00
Owner
Copy Link

Looks like this now has conflicts with master. I'm happy to merge it once that's fixed! (Alternatively, I can cherrypick it to master, but I want you to get "credit" for the work.)

Looks like this now has conflicts with master. I'm happy to merge it once that's fixed! (Alternatively, I can cherrypick it to master, but I want you to get "credit" for the work.)
witten approved these changes 2019-05-14 16:54:47 +00:00
decentral1se commented 2019-05-14 17:00:33 +00:00
Author
Contributor
Copy Link

:)

Rebased!

:) Rebased!
🎉 1
witten closed this pull request 2019-05-14 17:02:37 +00:00
decentral1se deleted branch cifix/isort-integration 2019-05-14 17:03:56 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#173
No description provided.
Delete Branch ":cifix/isort-integration"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?