Feature: Add Verb-Prelude property to load config files for requests

[jeff-phil]

• Add ability to set, load and parse external config files into verb-var's
• Update tests to validate properties and requests
• Update README.md for user documentation on the feature
• Minor updates to test harness
• Rebase of previous commit with sync-up of origin/main

[jeff-phil]

@federicotdn-

Please let me know if you have any questions, comments or feedback for the PR.

Thanks!

[federicotdn]

Hi! Sorry I did not have enough time the past weeks to look into this.

Unfortunately the way I was planning on implementing this was quite different. I want to allow users to load Elisp files instead of JSON - they can write their JSON-loading functionality, if they want, but the core feature should use Elisp, which is more flexible.

I was also planning on calling it Prelude instead of Env (as it fits better for a piece of code that gets executed before performing an action). And I also wanted to just have it be a document-level buffer setting (https://orgmode.org/manual/In_002dbuffer-Settings.html). This is similar to how #+FILETAGS is used for tagging the entire headings tree with :verb: automatically.

(so the things I had mentioned in https://github.com/federicotdn/verb/discussions/68 mostly)

I also am not planning on bumping the dependency on Emacs (for now at least, but probably in the near future).

Sorry if this is dissapointing! I do really appreciate you opening a PR here (of course), it's just that I was imagining it in a different way.

[jeff-phil]

Hi! Sorry I did not have enough time the past weeks to look into this.

Never a worry, just wanted to check and see.

I've made the requested changes, but not committed yet - since I wanted to just verify a few things prior. Hopefully you are okay with a little debate on the topics. If not, let me know! ...

I want to allow users to load Elisp files instead of JSON - they can write their JSON-loading functionality, if they want, but the core feature should use Elisp, which is more flexible.

Just to make sure you saw, the PR here includes functionality for loading Elisp or JSON determined automatically based on file content. This adds even more flexibility than just Elisp alone. And, as you know for web services, JSON is generally more common. But let me know if you really want to remove those 10 lines, and I will happily do that.

I also wanted to just have it be a document-level buffer setting ... This is similar to how #+FILETAGS is used for tagging the entire headings tree with :verb: automatically.

Sorry, I missed functionality for looking for document-level properties. But are you okay with flexibility of having lower-level heading override higher-level hierarchy? Reason is much like overriding template, Verb-Store, etc. I can see having things like a DEV, TEST, PROD section of a Verb Org file, where you need different variables, settings, etc. to happen based on if going against DEV, TEST, or PROD. Example would be things like having a common username at the buffer level, then having a different password for DEV, TEST and PROD. If you only want document-level, then let me know and I can remove the part that walks the hierarchy and just look at document properties.

I also am not planning on bumping the dependency on Emacs (for now at least, but probably in the near future).

I switched to just standard json.el, so Emacs dependency is back to 26.3.

Sorry if this is dissapointing! I do really appreciate you opening a PR here (of course), it's just that I was imagining it in a different way.

Not disappointing at all. I appreciate your time to feedback and let me bounce a few things back if you're open to it. At the end of the day, it is your package and your vision. I personally needed the functionality pretty quick for security and separation of concerns reasons, and wanted to help out if I could.

No rush, just let me know thoughts on the two questions I posed above.

[federicotdn]

Just to make sure you saw, the PR here includes functionality for loading Elisp or JSON determined automatically based on file content. This adds even more flexibility than just Elisp alone. And, as you know for web services, JSON is generally more common. But let me know if you really want to remove those 10 lines, and I will happily do that.

That is true, I misread the code. I would still vouch for only implementing an Elisp version since the code assumes that the JSON file will have a specific structure, which makes it a bit less useful. We can also load the Elisp file using the load function to avoid having to read it into a string etc.

Sorry, I missed functionality for looking for document-level properties. But are you okay with flexibility of having lower-level heading override higher-level hierarchy? Reason is much like overriding template, Verb-Store, etc. I can see having things like a DEV, TEST, PROD section of a Verb Org file, where you need different variables, settings, etc. to happen based on if going against DEV, TEST, or PROD. Example would be things like having a common username at the buffer level, then having a different password for DEV, TEST and PROD. If you only want document-level, then let me know and I can remove the part that walks the hierarchy and just look at document properties.

I see your point; it is true that it would be more useful like you describe it. Lets then allow the property to be set in any heading, in any case the behaviour I suggested can be implemented by just adding the property to the top level heading(s). Note though that for all this to work, org-use-property-inheritance needs to be set to t, which by default it's not. I've mentioned this in docs already though, since other functionality also depends on it.

I'll add some comments to the code now; I don't see any new commits pushed though 🤔

[federicotdn]

LMK if you need help with getting the tests to pass !

[jeff-phil]

Just to make sure you saw, the PR here includes functionality for loading Elisp or JSON determined automatically based on file content. This adds even more flexibility than just Elisp alone. And, as you know for web services, JSON is generally more common. But let me know if you really want to remove those 10 lines, and I will happily do that.

That is true, I misread the code. I would still vouch for only implementing an Elisp version since the code assumes that the JSON file will have a specific structure, which makes it a bit less useful. We can also load the Elisp file using the load function to avoid having to read it into a string etc.

Would you change your mind if I told you there is not a specific structure for JSON inclusion? JSON is read as a plist and then each key-value pair for the plist becomes the Verb variables. As long as it's valid JSON, it "Just Works ™️". If you still say no, I won't ask again. I just feel this is a feature that your users will prefer, since many may not know Elisp - but if using REST services, they know JSON. ;)

Sorry, I missed functionality for looking for document-level properties. But are you okay with flexibility of having lower-level heading override higher-level hierarchy? Reason is much like overriding template, Verb-Store, etc. I can see having things like a DEV, TEST, PROD section of a Verb Org file, where you need different variables, settings, etc. to happen based on if going against DEV, TEST, or PROD. Example would be things like having a common username at the buffer level, then having a different password for DEV, TEST and PROD. If you only want document-level, then let me know and I can remove the part that walks the hierarchy and just look at document properties.

I see your point; it is true that it would be more useful like you describe it. Lets then allow the property to be set in any heading, in any case the behaviour I suggested can be implemented by just adding the property to the top level heading(s). Note though that for all this to work, org-use-property-inheritance needs to be set to t, which by default it's not. I've mentioned this in docs already though, since other functionality also depends on it.

Regarding org-use-property-inheritance, since I walk up the hierarchical tree using org-up-heading-safe and getting Verb-Prelude at each level, then org-use-property-inheritance doesn't come into play. I think this is a bit different than reason org-use-property-inheritance was initially conceived, for performance on searching properties on large and very hierarchical traditional org files. A couple of options, if you would let me know your choice:

1. Leave as-is and keep documentation that Verb-Prelude will always inherit from parent if not set in children - but children override parents settings if set both places. Add that org-use-property-inheritance is not used for Verb-Prelude. Then wait for user feedback.
2. Add in capability for using org-use-property-inheritance for Verb-Prelude.

I'll add some comments to the code now; I don't see any new commits pushed though 🤔

Thanks for comments - I did already have tests and lints issues fixed. I was waiting to commit after I heard back from you. But I will go ahead and commit to ensure you see the latest I have.

[jeff-phil]

LMK if you need help with getting the tests to pass !

The tests should all pass, at least they do locally for me. I did need to add setuptools to the pip install in the Makefile for the venv due to tracerite/html.py importing pkg_resources.

And looks like issue with compat package installing, likely due to gnu.org having issues lately?

Anyway, not sure when they trigger here, or if you have the magic button to trigger the workflow. I don't have access to start that I can find. :)

[federicotdn]

Would you change your mind if I told you there is not a specific structure for JSON inclusion? JSON is read as a plist and then each key-value pair for the plist becomes the Verb variables. As long as it's valid JSON, it "Just Works ™️". If you still say no, I won't ask again. I just feel this is a feature that your users will prefer, since many may not know Elisp - but if using REST services, they know JSON. ;)

That is true, I guess that the worst case will be that the JSON will be loaded and none or only a subset of the desired values will be found. This is still useful. What I would change from verb-load-prelude-file though is to just use file extensions to determine loading strategy, I think the parsing of contents is less clear + a bit more brittle. WDYT? (note that using file extensions would then make using load a better option).

Regarding org-use-property-inheritance, since I walk up the hierarchical tree using org-up-heading-safe and getting Verb-Prelude at each level, then org-use-property-inheritance doesn't come into play. I think this is a bit different than reason org-use-property-inheritance was initially conceived, for performance on searching properties on large and very hierarchical traditional org files. A couple of options, if you would let me know your choice:

1. Leave as-is and keep documentation that `Verb-Prelude` will always inherit from parent if not set in children - but children override parents settings if set both places.  Add that `org-use-property-inheritance` is not used for `Verb-Prelude`.  Then wait for user feedback.

2. Add in capability for using `org-use-property-inheritance` for `Verb-Prelude`.

Ooh right yes, Verb-Prelude is interesting in that sense, it doesn't behave like other properties due to how verb-load-prelude-files-from-hierarchy is written, I hadn't catched that. Honestly for me both solutions work, though I prefer number 2 slightly to avoid having to explain more things (exceptions) to the user. If you want to add that functionality feel free to do so, otherwise we can document as in option 1.

For the review itself: looking good so far ! Added some more comments. Please pull from main since I've done some changes on that branch in the last days. But yeah it looks like gnu.org had some problems recently; I believe it's all working normally now and the tests should run correctly.

[jeff-phil]

That is true, I guess that the worst case will be that the JSON will be loaded and none or only a subset of the desired values will be found. This is still useful. What I would change from verb-load-prelude-file though is to just use file extensions to determine loading strategy, I think the parsing of contents is less clear + a bit more brittle. WDYT? (note that using file extensions would then make using load a better option).

Yes, agree, it needed another iteration of re-work. Since have potential of GPG encrypted files, that end in .gpg it is a bit more tricky - but have it changed now. For Emacs Lisp, I used load-file, since load expects load-path. Same thing in the end. I also included zipped files.

Ooh right yes, Verb-Prelude is interesting in that sense, it doesn't behave like other properties due to how verb-load-prelude-files-from-hierarchy is written, I hadn't catched that. Honestly for me both solutions work, though I prefer number 2 slightly to avoid having to explain more things (exceptions) to the user. If you want to add that functionality feel free to do so, otherwise we can document as in option 1.

I ended up with option 1. Since org-use-property-inheritance isn't just boolean, but can be a list of specific properties, etc. and gets a bit convoluted quickly. It should be fine, since there are other Org properties that also don't use it. I did document this more in the Readme.

Added some more comments. Please pull from main since I've done some changes on that branch in the last days.

Should all be synced up with latest in main and more comments. I did not change change log, etc. not sure what point you normally do that. Let me know if you want me to do that.

For the review itself: looking good so far !

Great! Thanks again for spending time reviewing and providing feedback back and forth.

[federicotdn]

Looks good! Thanks for adding this. I am a bit neurotic when it comes to README formatting/content so I will probably modify the examples provided, but it will still remain structurally the same.

For future reference, the tests for this feature did not really require server-test since this is more of a request spec parsing thing, so we don't really need to make actual HTTP requests to the testing server. However since you added tests (thanks 🙏🏻 ), and they do test the functionality, I much rather have them since they are still quite useful.

I can take care of the changelog btw.

Will merge this v soon.

[jeff-phil]

I am a bit neurotic when it comes to README formatting/content

That is a good thing! Thanks for taking time to fix up.

I much rather have them [the tests] since they are still quite useful.

Yes, running those each time before committing put me at much more ease that I didn't mess something up new or existing.

I can take care of the changelog btw.

Will merge this v soon.

Thanks for your fantastic package, and guiding me on this PR. Hopefully wasn't more trouble than worth for you.