-
Notifications
You must be signed in to change notification settings - Fork 476
Merge elpy snippets #278
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
Merge elpy snippets #278
Conversation
Elpy now ships with a few YASnippet snippets for Python. Fixes AndreaCrotti#43.
The enter and exit snippets now conform to the yasnippet-snippets repository (mostly), while super now makes use of the .yas-setup.el mechanism and is seriously improved. There's a new snippet for __init__ methods, too, which automatically assigns arguments to instance variables.
Previously when using the yasnippet "super", it will always return "(class, arg).method" which was the Py2 way of doing it. In Py3, you could simply do "().method" and Python will understand.
Thanks to galaunay for the idea to use this to "avoid unnecessary keyspace pollution."
Please note that python-interpreter must be set to "python3" for this to function correctly. As a side-effect it also allows the use of Python 2 on systems that install version 3 to /usr/bin/python and version 2 to /usr/bin/python2.
I suggest not to put |
Thanks to Andrea Crotti for the review and suggestion.
> Change elpy prefixes to yas-snips
>
> Thanks to @AndreaCrotti for the review and suggestion.
I suggest not to put @user into commit messages. Some people track
packages directly from git, and if they happen to push their forks to a
GitHub repo, then this will send a notification.
Oh my, I wish someone had mentioned this back in April... So to fix
this, I can use:
1) The sledgehammer reword commit which rewrites all subsequent
commits. The only upsides are that I can re-sign all GPG signed
commits would, that it's easy, and that the Elpy history remains
untouched.
2) Do #1 only up until the point of @galaunay's commits, and then
asking him to rebase...again. @galaunay, do you think this be too OCD?
haha Please let me know if you're ok with #1, and I'll go ahead and
do it asap.
Cheers,
Nicholas
|
@sten0 no problem for me with 1). |
6654b43
to
1cf482a
Compare
@galaunay thanks, that was much easier :-) Personally I'm a fan of more granular commits, especially when they reveal multiple issued I missed! I'll leave it up to you to find the squashed commit; that one was avoidable, with a worst-case scenario of using magit to ediff the unstaged file (conflict) during rebase...I didn't need to ediff, so its existence seemed strange ;-) @npostavs, By the way, thank you for explaining why |
Yeah, speaking as someone who's been on the receiving end of this, it was kind of annoying, although there weren't that many notifications in the end. I could imagine on some more popular repo it has the potential to be a real pain. (it might not be every time someone pushes, maybe only if they rebase and hence produce a "new" commit with the same message) |
d88ba48
to
6828266
Compare
That makes sense, and I fixed it. Is there any chance this PR could be merged before August? |
Yes sure I'm back from holidays now and I'll have another look in the next few days @sten0 |
@AndreaCrotti Hope you wonderful holidays :-) Does the seen label mean it will be merged soon? |
Well @sten0 it's just something I made up to check if there is something new, not necessarily that I'll merge soon, but sure I'd like to get this merged ASAP. |
@AndreaCrotti, that makes sense. I'm looking forward to the merge, so I can wrap up the Elpy side of this transfer of snippets :-) |
Hm, I just noticed Elpy uses python-shell-interpreter rather than python-interpreter and it would be nice to inherit that. Not worth blocking this PR though... If you'd like, feel free to add this. [15 Aug edit: Looked into it some more. Elpy's |
Hi @AndreaCrotti! How did the testing go? |
Hi @sten0
Even though it's weird the changes you've done should not have made it stop working, can you maybe check if you get the same error on that snippet please? |
|
||
(add-hook 'python-mode-hook | ||
'(lambda () (set (make-local-variable 'yas-indent-line) 'fixed))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a mention of this removal in the commit messages. Is it an accident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The silent removal is accidental. I seem to remember seeing this add-hook
while ediffing, and thinking something like "that's bad style and should be done differently elsewhere", and so I think I must have pressed b
and let it get overwritten...which is arguably a mistake of laziness... IIRC it is possible to inherit python-mode
's indentation style, which should already be 'fixed
, without using a hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreaCrotti, sorry for the delay. Yes, late last month I was able to reproduce it but I yet had the time to debug. The Elpy copy works, and assuming the yasnippet-snippets HEAD I was working off also works the only possibilities are: 1) I made a mistake in how I introduced configurable Python version in one of my last commits 2) I made a mistake while ediffing the conflicts during the merge 3) @galaunay might have introduced a regression. I think 1) and 2) are most likely!
|
||
(add-hook 'python-mode-hook | ||
'(lambda () (set (make-local-variable 'yas-indent-line) 'fixed))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The silent removal is accidental. I seem to remember seeing this add-hook
while ediffing, and thinking something like "that's bad style and should be done differently elsewhere", and so I think I must have pressed b
and let it get overwritten...which is arguably a mistake of laziness... IIRC it is possible to inherit python-mode
's indentation style, which should already be 'fixed
, without using a hook.
yas-snips-snippet-init-assignments should use values provided by yas--apply-transform instead of grabbing them itself. Thanks to npostavs for finding this issue. See review discussion at PR AndreaCrotti#278 for more info (search for "yas--apply-transform").
It's definitively time to get this merged 👍 |
Thanks @AndreaCrotti ! You noted #278 (comment), and I think this table of duplicate (possibly conflicting?) functionality might be an outstanding issue: #259 (comment) <- probably not a contributing factor to that class [snippet] error, but it seems like it could be a "requires maintainer action" kind of thing. #278 (comment) <- doubtful, but maybe that overwrite/deletion contributes to the class snippet error? That's everything I'm aware of at the moment. Sorry I'm not able to do more at this time (insane work schedule). |
Ok then I'm getting a lot of conflicts now in this PR now and it's a bit hard to merge it all at once. I think what I'm going to to do is that I'll pick all the snippets that have no Elisp dependencies first (which are quite a lot anyway) and merge them. Would that be ok @sten0 ? |
@AndreaCrotti, yes, that's fine, please go ahead; it sounds like the best way forward. Once again, sorry I can't yet help out more at this point in time (any free time for this kind of work is going towards Debian bugfix/project deadlines/freeze stuff). Post-March, I can diff master against https://github.com/sten0/yasnippet-snippets/tree/elpy-snippets to check for missing/conflicting functionality. Of course anyone else is welcome to do so in the meantime... |
This PR needs love. How could I help? I hardly read elisp, and never contributed snippets, but if help is needed Python side, I'd gladly help. From what I see:
Some may need to be fixed, some may just wait for another PR.
looks like a good strategy to me, would you like me to prepare such a PR with my comments applied? |
Hi Julien,
Julien Palard ***@***.***> writes:
This PR needs love. How could I help? I hardly read elisp, and never contributed snippets, but if help is needed Python side, I'd gladly help.
Please feel free to take over this PR. I'm sure your work will be
accepted when you balance the principle of least surprise (established
users) with new user expectations and the increasingly urgent need to
reflect updates to Python well as current conventions.
[I've ranked your work according to my possibly subjective priorities]
- `py3` could be removed, it's only usefull in Python 2 code, which
has less and less users (see
https://git.afpy.org/mdk/python-versions), same for `unicode_literals`
and `with_statement`.
- `enc` may be removed, UTF-8 is the default file encoding since Python 3.
- `__coerce__`, `__long__`, `__unicode__`, `__nonzero__`, `__oct__`,
`__metaclass__` and `__hex__` can be removed (were removed in Python
3.0).
+1 It's been three years since Python 2 was sunset!
- There's two __eq__, one in group `dunder methods` and one in group `Special methods`.
- `__get__` should have `owner=None` even if in CPython it's never the
case (as far as I know), anyway it's documented as is, let's just
stick to the doc.
+1 and feel free to choose whichever one seems more consistent to your
plan.
- There's a conflict between `__getattribute__` and `__getattr__`:
they both have the same key.
- the `pdb` can generate `breakpoint()` instead of `import pdb;
pdb.set_trace()`, samefor `tr`, I think one could be removed.
- There's `_repr` and `repr`, let's only keep `_repr` for consistency, same for `__str__`.
- There's two `try` keys.
Oh yeah, the required deduplication!
- the key for `doctest` (being `doc`) is misleading.
+1
- it misses __set_name__, maybe a few others but again it could be
added in future PRs, I don't want to see this one blocked for this.
Good call, yes, I agree that a second PR would be the best approach.
- `class` could get an `**kwargs` passed to super for better
collaboration (see
https://rhettinger.wordpress.com/2011/05/26/super-considered-super/).
I'd put this in the second PR.
- There's a few simple quotes, while the concensus seems to shift to
double quotes (+ black users) (see `git grep "'"`)
I'd also put this in the second PR.
Some may need to be fixed, some may just wait for another PR.
> I think what I'm going to to do is that I'll pick all the snippets that have no Elisp dependencies first (which are quite a lot anyway) and merge them.
looks like a good strategy to me, would you like me to prepare such a PR with my comments applied?
When you submit the PR, would you please reference this one (#278)?
Andrea did want to merge my PR, but then I ran out of time and
motivation.
Cheers,
Nicholas
|
Ah yes @JulienPalard if you want to take care of this it would be great, I'm not using Python anymore so I'm probably not the best person to judge every single snippet, but if everyone is happy I'll be happy to merge |
* elpy-initialize-variables: Add the Elpy snippet directory. Elpy now ships with a few YASnippet snippets for Python. Fixes #43. * Small rework of the snippets. The enter and exit snippets now conform to the yasnippet-snippets repository (mostly), while super now makes use of the .yas-setup.el mechanism and is seriously improved. There's a new snippet for __init__ methods, too, which automatically assigns arguments to instance variables. * Rework and expand the snippet collection. * Add __bool__ snippet. * updated yasnippet variable 'text' to 'yas-text' * Return correct form of "super" depending on Python version Previously when using the yasnippet "super", it will always return "(class, arg).method" which was the Py2 way of doing it. In Py3, you could simply do "().method" and Python will understand. * Move yasnippet-snippets onto their own branch * PR #259 Change keys for __foo__ from "foo" to "_foo" Thanks to galaunay for the idea to use this to "avoid unnecessary keyspace pollution." * Fix bug due to passage from elpy to yasnippet * Use snippet expansion for foo -> __foo__ * Fix bug when specifying arguments on several lines * Allow detection of Python 3 on PEP 394-compliant systems. Please note that python-interpreter must be set to "python3" for this to function correctly. As a side-effect it also allows the use of Python 2 on systems that install version 3 to /usr/bin/python and version 2 to /usr/bin/python2. * Change elpy prefixes to yas-snips Thanks to Andrea Crotti for the review and suggestion. * Add newline at end of file for all files that were missing one. * Make python snippets able to handle arguments with type annotations. * Use start-point from yas--apply-transform in snippet-init-assignment yas-snips-snippet-init-assignments should use values provided by yas--apply-transform instead of grabbing them itself. Thanks to npostavs for finding this issue. See review discussion at PR #278 for more info (search for "yas--apply-transform"). * Removing Python 2 snippets, it's EOL since 2020-01-01. * Some consistenty in group names. * Removing a duplicate. * Owner is documented as optional. cf. https://docs.python.org/3/reference/datamodel.html#object.__get__ * Resolve a conflict between _getattr and _getattribute * Deduplicate: it already exists in master (named pdb). * Dedup: already exists as __repr__ * Dedup: already exists as __str__ * fix: try is already used by ./try * fix misleading key. * Consistency with other dunders. * Consistency with other naming. * Removing super, we don't have all needed elisp yet. * Deduplicate two keys. * s not needed. * The hook does not works for me. * Add few missing newlines at end of file. --------- Co-authored-by: Jorgen Schaefer <forcer@forcix.cx> Co-authored-by: Jorgen Schaefer <contact@jorgenschaefer.de> Co-authored-by: Daniel Wu <dawu@wgen.net> Co-authored-by: Daniel Gopar <gopardaniel@yahoo.com> Co-authored-by: Nicholas D Steeves <nsteeves@gmail.com> Co-authored-by: galaunay <gaby.launay@tutanota.com>
Hi @AndreaCrotti and @galaunay!
This PR supersedes #259. Please refer to #259 for the full history and discussion. Also, please do not squash or rebase, because I took extra care to maximally preserve committer history ;-)
If there is anything left to do for this PR it would be resolving the duplicate functionality noted in #259 (comment)
...but that can be left for another day!
Cheers,
Nicholas