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

[craftedv2beta] locate-library-guard for crafted-writing-config, Documentation for crafted-writing #329

Merged
merged 2 commits into from
Aug 20, 2023

Conversation

sthesing
Copy link
Contributor

As in #313

@sthesing sthesing changed the title require-guard for crafted-writing-config [craftedv2beta] require-guard for crafted-writing-config Aug 16, 2023
@sthesing
Copy link
Contributor Author

I forgot to comment a question for this PR. It works now, but the hook calling the function behind the guard is redundant since the guard does the same thing: loading pdf-tools.
Would it make more sense to use locate-library, here, even if it is a package that needs to be required?
Or does it make more sense to remove the function (and the hook)?
I'm unsure.

@jvdydev
Copy link
Contributor

jvdydev commented Aug 17, 2023

I recall there was some issue when requiring pdf-tools directly it wants to install dependencies on the host.
Also if require is used to check, the function inside would be (as you pointed out) quite useless.

With locate-library + function/hook, this would basically (optionally) lazy-load, however I wonder if this requires a guard at all.
If there is no pdf-tools specific code run during this block, we might as well remove it (in my mind, anyway).

@sthesing
Copy link
Contributor Author

sthesing commented Aug 17, 2023

Yeah, pdf-tools is special in that it provides a function called pdf-tools-install which compiles the backend of the package. I think the code as present is intended to work like this:

  1. In crafted-writing-packages : don't install the package. Instead, provide a function called crafted-writing-install-pdf-tools which does nothing but adding pdf-tools to package-selected-packages.
  2. In crafted-writing-config: if the package is installed, wait for the user to go into doc-view mode for the first time. When they do, load the package and run the command pdf-tools-install.

So I guess this is best achieved by using locate-library. If you agree, I'll adapt the PR.

Edit: Come to think of it, if the crafted-writing-install-pdf-tools is to be called by the user, it needs to be interactive and needs to either run package-install-selected-packages or package--save-selected-packages.
I also wonder which is more in line with the project's philosophy:

  1. Either: Define the command crafted-writing-install-pdf-tools and tell the interested user in the docs to run it.
  2. Or: Tell the interested user in the docs to put (add-to-list 'package-selected-packages 'pdf-tools) into their init.el.

I tend to think the second option is preferable. What's your take, @jvdydev ?

@jvdydev
Copy link
Contributor

jvdydev commented Aug 17, 2023

I personally lean towards the second one as well.
I am also in favor of doing the same to the configuration side (instead of delaying installation).
Maybe I'm missing something, but I fail to see the reason for the delayed install?

This might also be a good time to discuss how we want to deal with things where we do want to provide customization in a module, but don't want to install the package by default (@jeffbowman).
With org-roam in crafted-org, we have the documentation provide an example configuration for org-roam, but users have to manually add it to their configuration.
Although I'm open for either (configuration in docs or extra configuration in the modules).

- pdf-tools: Remove installation script, provide documentation, instead.
- fixed a few docstrings in `crafted-writing-config`
@sthesing
Copy link
Contributor Author

I modified the PR. View it as a proposal, I'm happy to adapt it as needed or to follow a different approach.

I have now kept hook to require pdf-tools as needed. The hook is only added when the package is installed (using locate-library in the guard).
I have removed the wrapper around the call to pdf-tools-install and have instead added information to the documentation so that the user can do so themselves.
And while at it, I wrote documentation for the crafted-writing module.

I realize this significantly widens the scope of this PR, but I thought this is the pragmatic way to go. I'm happy to split this up into smaller PRs, just say the word.

P.S: Because of the regenerated info files, this will clash with my other documentation PR #331. I'll happily adapt either one to make merging easier. So if one of them is ready to be merged, just do it, I'll then amend the other.

@sthesing sthesing changed the title [craftedv2beta] require-guard for crafted-writing-config [craftedv2beta] require-guard for crafted-writing-config, Documentation for crafted-writing Aug 20, 2023
@sthesing sthesing changed the title [craftedv2beta] require-guard for crafted-writing-config, Documentation for crafted-writing [craftedv2beta] locate-library-guard for crafted-writing-config, Documentation for crafted-writing Aug 20, 2023
@jeffbowman
Copy link
Contributor

I still need to review the updates, but just as an initial reaction to the comments here:

  • The ideas presented were from another PR by someone who recommended the configuration for writers.
  • pdf-tools can be problematic in certain cases because of the dependency on a compiler.

I'm okay with option 2 above. Now, I'll go review the code and provide additional feedback as necessary.

Copy link
Contributor

@jeffbowman jeffbowman left a comment

Choose a reason for hiding this comment

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

Changes look good to me, anything else we need to consider before merging? (@jvdydev )

@jvdydev
Copy link
Contributor

jvdydev commented Aug 20, 2023

Looks good to me (=> ready for merge).

The docs formatting looked like it had a ton of nesting, but it renders fine in my info buffer, so I think this is fine (if we find out later, it's not a massive docs refactor either).

@jeffbowman jeffbowman merged commit b1a3764 into SystemCrafters:craftedv2beta Aug 20, 2023
@sthesing sthesing deleted the v2b-writing-guards branch August 20, 2023 23:35
@jvdydev jvdydev mentioned this pull request Aug 23, 2023
17 tasks
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