feature: add random verse querying #20

zyriab · 2023-09-21T20:15:57Z

Hey, as we talked about in #19 :)

You can just append random behind any supported command like:
BibleVerse random
BibleVerseInsert random
BibleVerse query random
etc...

I'm not really familiar with Lua so I'm sorry in advance if my code seems a bit weird!

It's all based on the KJV but I checked the behavior of the plugin and it should work without problem, even if the selected translation has less books or verses. If it has more, though (so called Apocryphe), they wont be selected.

I decided to use a file-scoped variable in order to keep my general modifications as small and clean as possible.
I think that, given the reduced size of the file and all that, it's perfectly okay to use a "global" here, feel free to contest in your review. 😛

Also, it seems that the diff affects the entire file, do you program on Windows? It's possible that our end of line character is not the same (programmed this on a Linux machine), sorry about that... I'm new to Neovim so I don't know how to check and how to fix that but if you have any info about that, I'll be happy to fix.
Other than that I set my editor to 3 tabs and used British English ("sanitise") in order to stay consistent with your style.

EDIT: Seems like I was mistaken and that you don't use 3 tabs! I set my editor to tabs of 4, all good now

Tell me if I missed anything.

Take care :)

anthony-halim · 2023-09-22T10:57:54Z

Hi, thank you for the PR!

I have reviewed your changes and I got the general idea. Given that the feature essentially adds a new command random without any user argument, an easier way to integrate the feature would be:

lua/bible-verse/core/init.lua

To keep it consistent with other commands, you can add a new function that handles querying random verses e.g. M.random_query instead of adding new arguments and modifying the other functions. This would minimise the changes needed and provide better separation of concerns.

The M.random_query can utilise your newly added functionality of getting a random bible verse and passing it to diatheke call and returning it.

lua/bible-verse/commands.lua

On M.setup 's M.commands, add a new entry random which calls the new core API M.random_query().

Lastly, purely optional, you can move the random-verse.lua into core folder :)

Again, thank you for your contribution and it looks great!

zyriab · 2023-09-22T11:30:47Z

Hey Anthony, thanks for the review! :)
I will do all of that once I got a moment after work :)

Do I have to do something else to make it work as insert or show?

anthony-halim · 2023-09-22T12:34:49Z

You can refer to lua/bible-verse/core/init.lua's M.query_and_show and M.query_and_insert respective on_submit function 👍 Most of the code in the function is just to ensure that the popup window is spawned with a sane height/width.

I'm not sure whether there is a good use case of inserting or showing random verses though; returning the random verse as is would better fulfill #19 as user would be able to call the API and integrate the output to other plugins more easily e.g. on splash screen.

zyriab · 2023-09-22T17:52:12Z

I just applied your comments, I hope all is good, it was a long day :)
I added a command to query and show a random verse as it's really a feature I'm looking to use but if you don't like it we can remove it and I'll implement it in a personal fork only.

anthony-halim · 2023-09-23T13:30:21Z

lua/bible-verse/core/init.lua

@@ -6,6 +6,7 @@ local Utils = require("bible-verse.utils")
 local Diatheke = require("bible-verse.core.diatheke")
 local Ui = require("bible-verse.core.ui")
 local Highlight = require("bible-verse.core.highlight")
+local random_verse = require("bible-verse.core.random-verse")


Rename to RandomVerse

anthony-halim · 2023-09-23T13:31:19Z

lua/bible-verse/core/init.lua

+	)
+	local popup_max_height = math.ceil(relative_size.height * popup_conf.size.max_height_percentage)
+
+	local query_result = M.random_query()


Need to pass in the popup_width such that the outputted format is correctly shown. Prefer to simply call process_query(...) instead of calling M.random_query() to avoid dependencies between public functions.

anthony-halim · 2023-09-23T13:44:17Z

lua/bible-verse/core/init.lua

@@ -110,4 +111,32 @@ function M.query_and_insert()
 	Ui:input(input_conf, on_submit)
 end

+--- Returns a random verse
+function M.random_query()
+	return process_query(random_verse(), Config.options.query_format, 0)


I'm not sure whether it would be easy to use M.random_query() if the format is using Config.options.query_format 🤔

A user who has set Config.options.query_format=bibleverse to get highlighting on the pop up will get difficult to read/use output from M.random_query().

Do you think it would be better to create a separate Config.options?

It's true, I haven't played enough with the config to know about this. I'll let you be the judge, it's probably a good idea :)

anthony-halim · 2023-09-23T13:46:27Z

lua/bible-verse/commands.lua

@@ -39,7 +46,7 @@ function M.setup()
 		local command = vim.trim(args.args or "")
 		M.cmd(command)
 	end, {
-		nargs = "?",
+		nargs = "*",


Doesn't seem that any of the new commands need to accept >1 arguments 🤔

Indeed, I forgot to change it back

anthony-halim · 2023-10-06T14:12:34Z

Hi @ZyriabDsgn, sorry for the late reply - life has been hectic for a bit.

Resuming the discussion, I think it is better to keep the feature focused on returning raw random verses (thus behaving almost like a library) and let user to extend from there. For example, user can integrate it as part of the splash screen, showing it as a Noice notification, or as a pop-up like what you intended.

Do you mind to revert the changes on pop-up with random verse? Again, thank you so much for your contribution!

P.S I would be happy to continue where you left off if you are too busy. The random verse do come in handy as a feature :)

zyriab · 2023-10-09T15:33:48Z

Hey @anthony-halim, sorry as well for the late answer and thanks for the heads up, life if pretty hectic over here as well!
I don't have much time to keep working on this feature for the moment so feel free to do what you think should be done :)
Otherwise I'll eventually get to it!

Take care and I hop all is well in your part of the world!

anthony-halim · 2023-10-12T03:13:55Z

Cool, I will get to it during the weekend 👍

anthony-halim · 2023-10-15T01:55:11Z

Hi, the feature has been merged on #21. I will add a follow up PR to update the README for the documentation.

Now public APIs accepts optional parameter: { query: string, random: boolean }. If the parameter is not supplied or random == false && query == "" it will fallback to the normal behaviour by prompting user input.

New API:

Query

require("bible-verse").query(query_opt?: { query: string, random: true })

Returns a parsed (but unformatted) Verse[] object, this is intended for user who wants to integrate with the API programmatically and format the output themselves. This should satisfy the use case of integrating via dashboards.

-- Sample usage: returns a random Verse[]
require("bible-verse").query({ random = true })

Query and Show

require("bible-verse").query_and_show(query_opt?: { query: string, random: true })

Query the verse and show it to the screen. This should satisfy your use case @ZyriabDsgn.

-- Sample usage: display random verse to the screen
require("bible-verse").query_and_show({ random = true })

Query and Insert

require("bible-verse").query_and_insert(query_opt?: { query: string, random: true })

Query the verse and insert it below the cursor in the current buffer. No use case yet, this is implemented just to ensure all APIs has consistent function signature.

-- Sample usage: insert random verse to current buffer
require("bible-verse").query_and_insert({ random = true })

Cheers!

zyriab · 2023-10-17T10:24:19Z

You rock :)

Arthur Wallendorff added 2 commits September 21, 2023 22:07

feature: add random verse querying

2b30c41

fix: formatting

a95d35a

Arthur Wallendorff added 5 commits September 22, 2023 18:24

chore: move file to proper folder

deec2a1

chore: remove unused function

826aec0

feature: add functions

1cd023a

chore: revert changes + implement newly added functions

cf424f3

chore: update readme

8f8e26e

anthony-halim reviewed Sep 23, 2023

View reviewed changes

anthony-halim mentioned this pull request Oct 15, 2023

Random verse? #19

Closed

anthony-halim closed this Oct 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add random verse querying #20

feature: add random verse querying #20

zyriab commented Sep 21, 2023 •

edited

Loading

anthony-halim commented Sep 22, 2023 •

edited

Loading

zyriab commented Sep 22, 2023

anthony-halim commented Sep 22, 2023

zyriab commented Sep 22, 2023 •

edited

Loading

anthony-halim Sep 23, 2023

anthony-halim Sep 23, 2023 •

edited

Loading

anthony-halim Sep 23, 2023

zyriab Sep 23, 2023

anthony-halim Sep 23, 2023

zyriab Sep 23, 2023

anthony-halim commented Oct 6, 2023

zyriab commented Oct 9, 2023

anthony-halim commented Oct 12, 2023

anthony-halim commented Oct 15, 2023

zyriab commented Oct 17, 2023

feature: add random verse querying #20

feature: add random verse querying #20

Conversation

zyriab commented Sep 21, 2023 • edited Loading

anthony-halim commented Sep 22, 2023 • edited Loading

zyriab commented Sep 22, 2023

anthony-halim commented Sep 22, 2023

zyriab commented Sep 22, 2023 • edited Loading

anthony-halim Sep 23, 2023

Choose a reason for hiding this comment

anthony-halim Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

anthony-halim Sep 23, 2023

Choose a reason for hiding this comment

zyriab Sep 23, 2023

Choose a reason for hiding this comment

anthony-halim Sep 23, 2023

Choose a reason for hiding this comment

zyriab Sep 23, 2023

Choose a reason for hiding this comment

anthony-halim commented Oct 6, 2023

zyriab commented Oct 9, 2023

anthony-halim commented Oct 12, 2023

anthony-halim commented Oct 15, 2023

zyriab commented Oct 17, 2023

zyriab commented Sep 21, 2023 •

edited

Loading

anthony-halim commented Sep 22, 2023 •

edited

Loading

zyriab commented Sep 22, 2023 •

edited

Loading

anthony-halim Sep 23, 2023 •

edited

Loading