Skip to content

Add recipe for orgstrap#7111

Merged
riscy merged 1 commit intomelpa:masterfrom
tgbugs:orgstrap
Sep 7, 2020
Merged

Add recipe for orgstrap#7111
riscy merged 1 commit intomelpa:masterfrom
tgbugs:orgstrap

Conversation

@tgbugs
Copy link
Copy Markdown
Contributor

@tgbugs tgbugs commented Sep 1, 2020

Brief summary of what the package does

orgstrap is a specification and tools for standalone self-bootstrapping org-mode files.

Direct link to the package repository

https://github.com/tgbugs/orgstrap

Your association with the package

Maintainer.

Relevant communications with the upstream package maintainer

Of course I know him. He's me!

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

@tgbugs
Copy link
Copy Markdown
Contributor Author

tgbugs commented Sep 1, 2020

Notes on the byte-compile warning and the package-lint errors.

The warning when byte compiling comes from a case where I am trying to reduce the code size and thus am leaving out anything that is not minimally necessary to get the code to function (like calls to ignore).

The package-lint errors are false positives. Both defuns in question are used temporarily in local variables and are otherwise in a quoted list. One is also included at top level, but it needs to retain a definition that is identical to the one used in the local variables since they are both derived from the same noweb block.

@tgbugs
Copy link
Copy Markdown
Contributor Author

tgbugs commented Sep 1, 2020

From the travis error log.

E: Failed to fetch http://us-east-1.ec2.archive.ubuntu.com/ubuntu/dists/xenial-updates/universe/binary-amd64/Packages.gz  Hash Sum mismatch

That's not something you want to see.

@tgbugs tgbugs mentioned this pull request Sep 2, 2020
@riscy
Copy link
Copy Markdown
Member

riscy commented Sep 6, 2020

No worries about Travis. A quick pass over the checklist:

orgstrap.el

byte-compile (using Emacs 27.1):

orgstrap.el:54:1:Warning: Unused lexical argument `fmt'
orgstrap.el:56:16:Warning: reference to free variable
    `org-coderef-label-format'
In orgstrap--trap-hack-locals:
orgstrap.el:285:55:Warning: assignment to free variable
    `orgstrap--local-variables'
In orgstrap--read-current-local-variables:
orgstrap.el:300:28:Warning: assignment to free variable
    `orgstrap--local-variables'
orgstrap.el:301:20:Warning: reference to free variable
    `orgstrap--local-variables'
In end of data:
orgstrap.el:465:1:Warning: the following functions are not known to be
    defined: org-babel-noweb-p, org-babel-expand-noweb-references,
    org-src-coderef-regexp, org-babel-goto-named-src-block,
    org-mark-ring-goto, org-babel-get-src-block-info, outline-next-heading,
    org-meta-return, org-babel-src-block-names, org-babel-find-named-block,
    org-babel-update-block-body

You can silence the warning about fmt by renaming it to _fmt and I think a handful of these are solved simply by adding (require 'org).

checkdoc (using version 0.6.2):

  • No issues!

package-lint (using version 20200901.2204):

2 issues found:
55:0: error: Define compatibility functions with a prefix, e.g. "orgstrap--org-src-coderef-regexp", and use `defalias' where they exist.
179:0: error: Define compatibility functions with a prefix, e.g. "orgstrap--org-src-coderef-regexp", and use `defalias' where they exist.

Other possible lints:

  • You have two URL's in your metadata block; one is incorrect
  • The constantsorgstrap--local-variable-eval-commands-0.x seem to add a lot of complexity. On a quick glance you seem to be supporting two API's which is laudable, but note Emacs has facilities for warning about deprecations that you could also consider using. You're also packing a lot of code inside these defconsts -- defining functions, unbinding functions (I don't see the benefit), and some slightly questionable variable manipulation (you setq-local org-confirm-babel-evaluate to some value, and then later you set it "back" to t, but it may not have been t` originally).
  • Some of your functions (defun orgstrap-install-orgstrap ()) just error with a "TODO" message. I'd suggest removing these and stowing them on a develop branch
  • orgstrap.el#L54: Consider unless ... instead of when (not ...)
  • orgstrap.el#L178: Consider unless ... instead of when (not ...)
  • orgstrap.el#L368: Consider unless ... instead of when (not ...)
  • You could opt to use an SPDX-License-Identifier which is slightly more formal than your current License: indicator. Not a blocker though.

@tgbugs
Copy link
Copy Markdown
Contributor Author

tgbugs commented Sep 7, 2020

@riscy Thank you very much for the review! I have pushed several updates.

No worries about Travis. A quick pass over the checklist:

orgstrap.el

byte-compile (using Emacs 27.1):

All should be fixed now.

checkdoc (using version 0.6.2):

* No issues!

Should still be the case

package-lint (using version 20200901.2204):

2 issues found:
55:0: error: Define compatibility functions with a prefix, e.g. "orgstrap--org-src-coderef-regexp", and use `defalias' where they exist.
179:0: error: Define compatibility functions with a prefix, e.g. "orgstrap--org-src-coderef-regexp", and use `defalias' where they exist.

So for this one I fixed the two warnings as suggested, and instead got 7 new warnings because I was
aliasing to org-src-coderef-regexp and it is yelling about the fact that I should be depending on Emacs 26.1 if I need that function ... except for the fact that the reason why I am doing this is explicitly to backport that function so that it is present for 24 and 25. Where are feature expressions when you need them?

Other possible lints:

* You have two URL's in your metadata block; one is incorrect

Thanks, fixed.

* The constants`orgstrap--local-variable-eval-commands-0.x` seem to add a lot of complexity.  On a quick glance you seem to be supporting two API's which is laudable, but note Emacs has facilities for warning about deprecations that you could also consider using.

I made some major changes around this because they weren't really 0.1 and 0.2, they were really "portable" and "minimal." The thing that needed to be versioned doesn't actually need to be versioned and is the normalization procedure, which just needs to be differentiated. I still stick a version on there just for simplicity sake, but there will only ever be one of those in the codebase at a time.

You're also packing a lot of code inside these defconsts -- defining functions, unbinding functions (I don't see the benefit), and some slightly questionable variable manipulation (you setq-local org-confirm-babel-evaluate to some value, and then later you set it "back" to t, but it may not have been t` originally).

I got rid of that unbinding, there was no reason to do it. I also moved all of the code inside function definitions so that it really is constant and cannot be easily changed. This is because that code is used to populate local variables and it needs to be tightly controlled since it is run via an eval local variable. In almost all cases it is deduplicated using noweb in the source in README.org.

Also darn! You caught my attempt to enforce safe code execution defaults on users! I have fixed it by storing the original value and restoring it in the unwind forms.

* Some of your functions `(defun orgstrap-install-orgstrap ())` just error with a "TODO" message.  I'd suggest removing these and stowing them on a develop branch

Axed all of those. They were an overly complicated brainstorming exercise.

* orgstrap.el#L54: Consider `unless ...` instead of `when (not ...)`

Done. I nearly always write it out as when not because my brain refuses to grok unless yet has no issue with when not, and then I forget to change it to unless.

* You could opt to use an [SPDX-License-Identifier](https://spdx.org/using-spdx-license-identifier) which is slightly more formal than your current `License:` indicator.  Not a blocker though.

Done. I should have known to do it since I have been promulgating the use of spdx for licenses amongst my colleagues for years (derp).

@riscy
Copy link
Copy Markdown
Member

riscy commented Sep 7, 2020

Thanks for accommodating!

It is yelling about the fact that I should be depending on Emacs 26.1 if I need that function

We can mark those as false positives now -- this looks like the right approach to me. While here I'll note it can be safer for your refactoring efforts (renaming functions, etc.) if you use a sharp-quote in your defaliases -- then the byte-compiler will warn you when the functions don't exist:

  • orgstrap.el#L86: It's safer to sharp-quote function names; use #'
  • orgstrap.el#L309: It's safer to sharp-quote function names; use #'
  • orgstrap.el#L330: It's safer to sharp-quote function names; use #'
  • orgstrap.el#L346: It's safer to sharp-quote function names; use #'
  • orgstrap.el#L377: It's safer to sharp-quote function names; use #'

I nearly always write it out as when not because my brain refuses to grok

This is fair, and it's definitely not a blocker - I'll actually try to soften the language around that "lint". In ruby for example I don't use unless for similar reasons to you, but in lisps it removes one level of nesting which is sometimes a (small) benefit.

SPDX

For what it's worth, I primarily see SPDX licenses written as ;; SPDX-License-Identifier: GPL-3.0-or-later which seems to be the recommended form. Not a blocker.

All that said, welcome to MELPA!

@riscy riscy merged commit 35f53a7 into melpa:master Sep 7, 2020
@tgbugs
Copy link
Copy Markdown
Contributor Author

tgbugs commented Sep 8, 2020

I made the changes to use the sharp form for aliases, I had wondered about whether that could be done before, but had never actually seen it used, so good to know there are benefits! For whatever reason I haven't figured out which tool produces the output with the warnings thought. I think reminding about unless is good, and the language used is fine, though the use of consider is probably interpreted as passive aggression by some readers (whether it is intended or not), and what I have learned over time is that reducing consing is supposedly always worth a little bit of confusion, so having a remind for things like that is a plus. I also fixed the spdx convention, and caught a couple of other bugs along the way.

Glad to be here. Thanks!

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.

2 participants