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 recipe for org-super-links #7318

Closed
wants to merge 1 commit into from
Closed

Conversation

toshism
Copy link

@toshism toshism commented Dec 29, 2020

Brief summary of what the package does

Create links in org-mode with auto backlinks is the general idea.

Direct link to the package repository

https://github.com/toshism/org-super-links

Your association with the package

author

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

  • The package is released under a GPL-Compatible Free Software License.
  • I've read CONTRIBUTING.org
  • I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
  • My elisp byte-compiles cleanly
  • M-x checkdoc is happy with my docstrings
  • I've built and installed the package using the instructions in CONTRIBUTING.org
  • I have confirmed some of these without doing them

@toshism toshism changed the title org-super-links Add recipe for org-super-links Dec 29, 2020
@riscy
Copy link
Member

riscy commented Jan 3, 2021

Thanks, looks useful. Here's a quick pass over the files --

org-super-links-org-ql.el

byte-compile (using Emacs 27.1):

org-super-links-org-ql.el:55:17:Warning: reference to free variable
    `helm-org-ql-actions'
org-super-links-org-ql.el:57:8:Warning: assignment to free variable
    `helm-org-ql-actions'
org-super-links-org-ql.el:60:17:Warning: reference to free variable
    `helm-org-ql-actions'
org-super-links-org-ql.el:60:17:Warning: assignment to free variable
    `helm-org-ql-actions'

checkdoc (using version 0.6.2):

  • No issues!

package-lint (using version 20200906.512):

3 issues found:
1:64: warning: You should depend on (emacs "24.1") if you need lexical-binding.
59:1: error: You should depend on (emacs "24.4") if you need `with-eval-after-load'.
59:1: warning: `with-eval-after-load' is for use in configurations, and should rarely be used in packages.

org-super-links-org-rifle.el

byte-compile (using Emacs 27.1):

org-super-links-org-rifle.el:45:17:Warning: reference to free variable
    `helm-org-rifle-actions'
org-super-links-org-rifle.el:45:17:Warning: assignment to free variable
    `helm-org-rifle-actions'
In org-super-links-org-rifle-link-search-interface:
org-super-links-org-rifle.el:49:17:Warning: reference to free variable
    `helm-org-rifle-actions'
org-super-links-org-rifle.el:51:8:Warning: assignment to free variable
    `helm-org-rifle-actions'

checkdoc (using version 0.6.2):

  • No issues!

package-lint (using version 20200906.512):

3 issues found:
1:67: warning: You should depend on (emacs "24.1") if you need lexical-binding.
44:1: error: You should depend on (emacs "24.4") if you need `with-eval-after-load'.
44:1: warning: `with-eval-after-load' is for use in configurations, and should rarely be used in packages.

org-super-links.el

byte-compile (using Emacs 27.1):

  • No issues!

checkdoc (using version 0.6.2):

Warning (emacs): 
org-super-links.el:43: Some lines are over 80 columns wide
Warning (emacs): 
org-super-links.el:64: Some lines are over 80 columns wide
Warning (emacs): 
org-super-links.el:92: Some lines are over 80 columns wide
Warning (emacs): 
org-super-links.el:124: Lisp symbol `org-refile' should appear in quotes
Warning (emacs): 
org-super-links.el:279: All interactive functions should have documentation
Warning (emacs): 
org-super-links.el:286: All interactive functions should have documentation
Warning (emacs): 
org-super-links.el:309: All variables and subroutines might as well have a documentation string
Warning (emacs): 
org-super-links.el:317: All variables and subroutines might as well have a documentation string
Warning (emacs): 
org-super-links.el:348: First line is not a complete sentence
Warning (emacs): 
org-super-links.el:348: Argument `arg' should appear (as ARG) in the doc string
Warning (emacs): 
org-super-links.el:348: Lisp symbol `org-mode' should appear in quotes
Warning (emacs): 
org-super-links.el:388: Some lines are over 80 columns wide

package-lint (using version 20200906.512):

23 issues found:
1:57: warning: You should depend on (emacs "24.1") if you need lexical-binding.
33:10: error: You should depend on (emacs "24.3") if you need `org-element'.
199:3: error: You should depend on (emacs "24.3") if you need `org-element-map'.
199:20: error: You should depend on (emacs "24.3") if you need `org-element-parse-buffer'.
201:22: error: You should depend on (emacs "24.3") if you need `org-element-property'.
212:5: error: You should depend on (emacs "26.1") if you need `org-next-visible-heading'.
219:18: error: You should depend on (emacs "24.3") if you need `org-element-at-point'.
221:14: error: You should depend on (emacs "24.3") if you need `org-element-type'.
222:21: error: You should depend on (emacs "24.3") if you need `org-element-property'.
230:16: error: You should depend on (emacs "24.3") if you need `org-element-property'.
235:22: error: You should depend on (emacs "24.3") if you need `org-element-property'.
235:57: error: You should depend on (emacs "24.3") if you need `org-element-property'.
259:12: error: You should depend on (emacs "26.1") if you need `org-log-beginning'.
264:2: error: You should depend on (emacs "24.3") if you need `org-indent-region'.
301:8: error: You should depend on (emacs "26.1") if you need `org-log-beginning'.
304:13: error: You should depend on (emacs "27.1") if you need `org-link-make-string'.
306:5: error: You should depend on (emacs "24.3") if you need `org-indent-region'.
365:18: error: You should depend on (emacs "24.3") if you need `org-element-property'.
365:47: error: You should depend on (emacs "24.3") if you need `org-element-context'.
366:9: error: You should depend on (emacs "24.3") if you need `org-element-property'.
366:36: error: You should depend on (emacs "24.3") if you need `org-element-context'.
384:33: error: You should depend on (emacs "24.3") if you need `org-element-context'.
411:13: error: You should depend on (emacs "24.1") if you need `org-capture'.

Other lints:

  • org-super-links.el#L313: It's safer to sharp-quote function names; use #'
  • org-super-links.el#L411: It's safer to sharp-quote function names; use #'
  • org-super-links.el#L411: Avoid adding advice on import; this may belong in your ;;; Commentary?
  • org-super-links.el#L410: Loading a package should rarely add advice

Loadability

Verifying ability to #'load each file:

Loading org-super-links-org-ql.el
Loading org-super-links-org-rifle.el
Loading org-super-links.el
Done.

Package

  • org-super-links-pkg.el -- consider excluding; MELPA creates one out of org-super-links.el
  • Package-Requires mismatch between org-super-links-org-ql.el and org-super-links-pkg.el
  • Package-Requires mismatch between org-super-links-org-rifle.el and org-super-links-pkg.el
  • Package-Requires mismatch between org-super-links.el and org-super-links-pkg.el

@riscy
Copy link
Member

riscy commented Jan 10, 2021

Feel free to ping when ready for another look!

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Jan 10, 2021
@toshism
Copy link
Author

toshism commented Jan 11, 2021

Thanks for taking a look @riscy . I guess it's ready for another look. I fixed the issues I see when running package-lint locally, but my results were also different from yours. Not sure where the discrepancy is coming from. package-lint version is 20200906.512 and emacs 27.1.

The changes are in this commit toshism/org-super-links@01fb732 on develop branch but I have not merged them to master branch yet. I was curious if you might know what is causing the difference in my results locally and yours before I merge.

The one remaining issue is

45:1: warning: `with-eval-after-load' is for use in configurations, and should rarely be used in packages.` 

but I expect that one and would prefer to leave it as is.

Thanks!

@riscy riscy removed the awaiting-upstream Awaiting action from an upstream maintainer label Jan 18, 2021
@riscy
Copy link
Member

riscy commented Jan 18, 2021

I was curious if you might know what is causing the difference in my results locally and yours before I merge.

I believe it's because I'm setting the package-lint-main-file variable. It's really only a problem if they differ, in which case only the package-requires in your main file will be used.

I expect that one and would prefer to leave it as is.

The reason package-lint warns about this is because we prefer packages don't modify Emacs's state simply when they are loaded. [If I include] org-super-links.el#L419 there are three places across three files where advice is being added that a novice user won't be able to easily undo. The idiomatic way to do that in this case would be to consolidate all three into a minor mode that is easy for users to turn off and on.

@riscy
Copy link
Member

riscy commented Feb 6, 2021

Just to follow up on this (and a friendly ping) I believe the advice-add should still be left for users to configure or toggle via a function you supply.

Maybe I can summon @alphapapa to double-check whether these are expected registration mechanisms for these packages:

(with-eval-after-load "helm-org-ql"
  (add-to-list 'helm-org-ql-actions '("Super Link" . org-super-links-org-ql-insert-link-action) t))

(with-eval-after-load "helm-org-rifle"
  (add-to-list 'helm-org-rifle-actions '("Super Link" . org-super-links-org-rifle-insert-link-action) t))

@toshism
Copy link
Author

toshism commented Feb 9, 2021

@riscy thanks for the ping. I've just been dragging my feet to make a decision on this. I'll do that soon.

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Feb 14, 2021
@riscy
Copy link
Member

riscy commented May 27, 2021

Friendly ping. :)

@alphapapa
Copy link
Contributor

Maybe I can summon @alphapapa to double-check whether these are expected registration mechanisms for these packages:

@riscy Sorry for the delay; haven't been keeping up with notifications.

I guess those are okay. I've never used them like that, and I haven't seen anyone do so, but I don't know of any reason it shouldn't work. (I might suggest cl-pushnew instead of add-to-list, but I guess that's a matter of preference.)

@riscy riscy removed the awaiting-upstream Awaiting action from an upstream maintainer label Jun 4, 2021
@riscy
Copy link
Member

riscy commented Jun 19, 2021

@toshism I'll mark this awaiting-upstream, there's no rush. (Thanks again @alphapapa for chiming in.)

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Jun 19, 2021
@tarsius
Copy link
Member

tarsius commented Feb 13, 2022

I am closing this and other pull-requests from 2020.

If/When you still want to add this package to Melpa, then please address the feedback as much as possible before asking us to reopen this pull-request and do another review.

Cheers!

@tarsius tarsius closed this Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants