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

Add pyramid #5451

Merged
merged 1 commit into from May 6, 2018
Merged

Add pyramid #5451

merged 1 commit into from May 6, 2018

Conversation

dakra
Copy link
Contributor

@dakra dakra commented Apr 25, 2018

As this is my first melpa package I would appreciate comments about code and style.
Even "nitpicky" and opinionated comments like "I always put 2 new lines between function definitions" or "Separate sections with ^L" are welcome :)

Brief summary of what the package does

Pyramid provides utilities for working with the python
web framework pyramid.

Direct link to the package repository

https://github.com/dakra/pyramid.el

Your association with the package

Maintainer

Checklist

Please confirm with x:

Pyramid provides utilities for working with the python
web framework pyramid.
recipes/pyramid Outdated
(pyramid
:repo "dakra/pyramid.el"
:fetcher github
:files ("*.el" "snippets"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

please replace "*.el" with :defaults:

:files (:defaults "snippets")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. thanks

@dakra
Copy link
Contributor Author

dakra commented Apr 28, 2018

Not sure if this is the right place to ask but I saw that when
my track pdb compilation filter matches and tries to start
inferior-python-mode it errors with void variable for
python-shell--interpreter and python-shell--interpreter-args.

That's why this commit is needed.
Just let binding them doesn't work either, I have to put the devfar
there and let bind them to nil.
Somehow that doesn't look right, am I doing something wrong or is it
even a bug in python.el?
Link to the relevant python.el section for the lazy.

@DamienCassou
Copy link
Collaborator

Some feedback:

  • you don't have to specify :group for each of your defcustom as this is inferred from the previous defgroup

  • regarding pyramid-project-root, I guess you can use the directory type instead of string.

  • docstrings typically contain the phrasing "non-nil" instead of "True".

  • you can specify that the type of pyramid-cookiecutters is a list of strings if you want to

  • if the variable pyramid-get-views-code keeps on growing, I would recommend moving its value to an external file. Python syntax-highlighting will make understanding easier. Same comment for the other python variable.

  • I typically split my code into sections, separated with ^L\n;;; Some title. Then you can use section-movement keybindings (such as C-x [, backward-page)

  • the function pyramid-call is a bit weird in my opinion:

    • I usually avoid using setq when a let is enough: I would move the value of output inside the let bindings and replace the let with a let* so the block can reference exit-code:
    (let* ((exit-code nil)
            (output (with-output-to-string ...))))
    • there is a call to hack-dir-local-variables-non-file-buffer but it's not clear which directory is being used as base one
    • I've never seen standard-output used that way. What about something like that (untested):
    (let* ((buffer (get-buffer-create "*pyramid-pythonic*"))
           (exit-code (call-pythonic :buffer buffer))
           (output (with-current-buffer buffer (buffer-string))))
      (when ()
        (pyramid-show-error output (format "Python exit with status code %d" exit-code)))
      output)
  • I find the docstring of pyramid-find-file-and-line not really clear. What is KEY? What is COLLECTION made of? You can say that COLLECTION is analist of (VAL1 . VAL2) where VAL1... and VAL2... for example (please use descriptive names instead of VALx).

  • please check if funcall would be more appropriate than apply

  • pyramid-find-file-and-line uses the variable value for two different things. I would use two variables.

@dakra
Copy link
Contributor Author

dakra commented Apr 30, 2018

@DamienCassou thank you for the thorough feedback.
I fixed almost all of you points 👍

there is a call to hack-dir-local-variables-non-file-buffer but it's not
clear which directory is being used as base one

As with-current-buffer changes the buffer, the dir-locals from the
user where he potentially sets the python interpreter is ignored.
So the hack-dir-local-variables-non-file-buffer uses the
directory of the file where the function was originally called.
See: pythonic-emacs/djangonaut#2
Do you think I should a comment there with explanation?

pyramid-find-file-and-line uses the variable value for two different
things. I would use two variables.

It's used for the same thing.
It only appends tramp specific things when the source file
is located on the network/docker/etc.

Thanks again.

@purcell
Copy link
Member

purcell commented May 6, 2018

Basically looks great, but just a couple of notes:

  • When you build commands using concat like here, you'll have a bad time if paths contain spaces or quotes. Use shell-quote-argument to avoid this.
  • When the minor mode is disabled, this line will cause the snippets to be loaded. You probably want to wrap that with (when pyramid-mode (pyramid-load-snippets)). But in any case, you almost certainly just want to load the snippets once, so consider (with-eval-after-load 'yasnippet ...) at the top level instead.
  • You can omit the 1 here - when called non-interactively, this is the default behaviour of mode functions.

In any case, this is ready to merge, and you can address the above separately. Welcome to MELPA!

@purcell purcell merged commit f786a47 into melpa:master May 6, 2018
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