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

minor tweaks needed for melpa approval #2

Closed
2 of 5 tasks
masukomi opened this issue Mar 30, 2022 · 1 comment
Closed
2 of 5 tasks

minor tweaks needed for melpa approval #2

masukomi opened this issue Mar 30, 2022 · 1 comment
Assignees

Comments

@masukomi
Copy link
Owner

masukomi commented Mar 30, 2022

Feeback on the PR to be included in melpa included the following suggestions

  • add SPDX or other license in the private-comments-mode.el file directly
  • suggestion to link directly to the private comments server repo in the commentary
  • convert some error calls to be user-error (see below 1)
  • comment about a progn (see below 2)
  • no space in a lighter (see below 3)

The two checked items are addressed in the melpa_tweaks branch.

Details on the unchecked items above. Quoted sections are from the feedback on the melpa PR

  1. re. this comment, it wasn't obvious to me which, error calls should be left as-is, so i just didn't change any rather than only making some of the necessary changes.

I think this error: (error "No private comment found") should be user-error; there are a few other places where I'd suggest replacing error with user-error such as in private-comments-delete and private-comments-record -- worth double-checking these (user-errors avoid spilling backtraces to the user)

  1. I think this suggestion is incorrect because, as i read it, the 2nd progn in the else but maybe i'm misinterpreting.

I believe the second progn in private-comments--apply-callback shouldn't be required as the "else" part of an if-let

  1. I checked the docs re this comment and I saw no mention of anything special about spaces so i have no idea. Also confused as to why this lighter and not the other one. Maybe he just didn't notice the other one.

I think the lighter :lighter " PCM Edit" should avoid having a space with it (spaces are how we usually distinguish between the different lighters)

@dickmao
Copy link
Collaborator

dickmao commented Mar 30, 2022

HI there. I made all the appropriate changes in 57eb1ba and also merged your melpa branch.

I confirm the suggestion regarding the 2nd progn is in error.

I confirm the suggestion regarding :lighter is also in error. The space is an unfortunate but unspoken hack well known to all minor mode writers because upstream emacs simply catenates the lighters, and not having the space results in ugliness like "Mode1Mode2" instead of "Mode1 Mode2"

@dickmao dickmao closed this as completed Mar 30, 2022
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

No branches or pull requests

2 participants