Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to edit for buffers visiting files #114

Closed
wants to merge 2 commits into from

Conversation

deb0ch
Copy link
Contributor

@deb0ch deb0ch commented Nov 4, 2017

As discussed in #113, implementing tests for this is still needed.

I tested it manually and

  • the buffers that usually open in 'edit are still in 'edit
  • *magit: blah* still gets 'general
  • if I create a file (SPC ff RET blablabla RET in Spacemacs it nicely gets 'edit
  • if I create a buffer (spacemacs/new-empty-buffer) with a random name it nicely gets 'general

@coveralls
Copy link

coveralls commented Nov 4, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling 5100ed4 on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

coveralls commented Nov 4, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling a238772 on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling 1cdc59a on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling 1cdc59a on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling 609408a on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling ea3c537 on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling 7bb45e4 on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling c401b4b on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@deb0ch
Copy link
Contributor Author

deb0ch commented Nov 5, 2017

Hahaha, what made the test fail previously was... the other test, not the commit itself.

It's all clean now, ready for proper review and merge (?).

Indeed, implementing the tests was a lot harder than implementing the feature 💦

Copy link
Owner

@bmag bmag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the PR, there's a few changes that need to be made still.

@@ -165,13 +165,16 @@ This allows Purpose to work well with both `ido' and `helm'.")

;;; Overloaded commands: (C-u to get original Purpose-less behavior)
(define-purpose-prefix-overload purpose-find-file-overload
'(purpose-friendly-find-file find-file-without-purpose))
'(purpose-friendly-find-file
find-file-without-purpose))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary formatting changes.


(define-purpose-prefix-overload purpose-find-file-other-window-overload
'(purpose-friendly-find-file-other-window find-file-other-window-without-purpose))
'(purpose-friendly-find-file-other-window
find-file-other-window-without-purpose))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary formatting changes.


(define-purpose-prefix-overload purpose-find-file-other-frame-overload
'(purpose-friendly-find-file-other-frame find-file-other-frame-without-purpose))
'(purpose-friendly-find-file-other-frame
find-file-other-frame-without-purpose))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary formatting changes.

@@ -212,6 +212,9 @@ If no purpose was determined, return `default-purpose'."
(purpose--buffer-purpose-mode buffer-or-name
purpose--default-mode-purposes)))

;; If the buffer is visiting a file, fallback to 'edit purpose
(when (buffer-file-name (get-buffer buffer-or-name))
'edit)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use a new custom default-file-purpose variable, which has a default value of edit.

(get-buffer-create "yolo")
(should (equal (purpose-buffer-purpose (get-buffer "yolo")) default-purpose))
(kill-buffer "yolo")))

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use unwind-protect and purpose-kill-buffers-safely. This ensures proper tear-down of the test, and separate the tear-down from what's actually being tested. See other tests in the same file for examples.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling b8ba90c on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage increased (+0.05%) to 72.527% when pulling 5942f70 on deb0ch:default-to-edit-for-files into 2b80592 on bmag:master.

@deb0ch
Copy link
Contributor Author

deb0ch commented Nov 5, 2017

All done 👌✨

@bmag
Copy link
Owner

bmag commented Nov 7, 2017

I squashed your commits and made some additional fixes in 2655bbe. Thank you.

@bmag bmag closed this Nov 7, 2017
@deb0ch
Copy link
Contributor Author

deb0ch commented Nov 7, 2017

Yay ! 🎉 🎊

That was a swift one, so good to see something merged so quickly right after initial discussion 😸

Nice work !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants