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

Add a variation of AbstractBlock#find_by that returns a single result #3135

Open
mojavelinux opened this issue Mar 8, 2019 · 6 comments
Open
Assignees
Milestone

Comments

@mojavelinux
Copy link
Member

When using find_by with an ID selector, or another unique characteristic, it seems odd that the method returns a collection instead of a single result. It should be possible to use the method in "singular" mode.

One idea is to pass the option :single_result.

table = doc.find_by context: :table, single_result: true

When the ID selector is specified, single_result: true would be implied:

block = doc.find_by id: 'the-one-and-only'

The only question is whether single_result should enforce that there's only one match, or whether it should simply pick off the first result in the collection.

We could follow the JPA approach and have single_result enforce and return a single result (or nil), and first_result return the first result in the collection.

first_table = doc.find_by context: :table, first_result: true

The other option is to introduce separate methods to handle the result filtering:

  • find_one_by - enforces and returns a single result or nil
  • find_first_by - returns the first result only
@ggrossetie
Copy link
Member

ggrossetie commented Mar 8, 2019

The other option is to introduce separate methods to handle the result filtering

I don't know if it's idiomatic Ruby but I always make a clear distinction between methods that return a collection and methods that return an (optional) single result:

def get_by # Option[Node]
def find_by # List[Node]

enforces and returns a single result or nil

JPA is throwing exceptions:

  • NonUniqueResultException - if more than one result
  • NoResultException - if there is no result

I think it's more idiomatic in Ruby to return nil If there is no result. And if there's more than one result, then I don't know... probably throwing an exception ? or return a "NonUniqueResult symbol" ?

The only question is whether single_result should enforce that there's only one match, or whether it should simply pick off the first result in the collection.

I'm leaning toward "enforce" because the user specifically asked for a single result so if there is more than one result it's "unexpected". And we definitely don't want to hide it under the carpet and pretend there is only one result.

find_first_by - returns the first result only

In my opinion this method has low value.

When the ID selector is specified, single_result: true would be implied:

I don't like this implicit because it changes the "return type".

@mojavelinux
Copy link
Member Author

In this case, I can't really comment on what's idiomatic Ruby or not. I'm more interested in what's intuitive.

Am I correct in saying that you're suggesting the following signature for the method that returns and enforces a single result?

def get_by selector, &block # Node

That seems reasonable to me.

I'm still wondering if we should have something that parallels querySelector and querySelectorAll from DOM. The querySelector method only returns the first match, whereas querySelectorAll returns a collection of all matches. Are we missing something if we don't have a parallel for that commonly-used API? I suppose we could add query and queryAll to match that behavior.

@ggrossetie
Copy link
Member

Am I correct in saying that you're suggesting the following signature for the method that returns and enforces a single result?

Yes

I'm still wondering if we should have something that parallels querySelector and querySelectorAll from DOM. The querySelector method only returns the first match, whereas querySelectorAll returns a collection of all matches. Are we missing something if we don't have a parallel for that commonly-used API? I suppose we could add query and queryAll to match that behavior.

The good thing is that we don't have to deal with the unexpected case (ie. when get_by returns more than one result). But then again query has low value, it basically returns the first match of queryAll (syntactic sugar).

document.querySelector('p') === document.querySelectorAll('p')[0]

document.querySelector('h5') // null
document.querySelectorAll('h5')[0] // undefined

I guess it depends on what we want to provide.
If we want a way to enforce a single result then we should aim for find_by and get_by otherwise query and query_all is fine.

@dsisnero
Copy link

in Ruby Enumerable module
find with alias detect is for singular return and

find_all with alias select is used for Array

doc.find{| doc| doc.id}

doc.select{|d| d.selector('p')}

@dsisnero
Copy link

doc.find{|d| doc.id == '#term'}

doc.select{|d| d.selector == 'p'}

doc.select_with_query('p')

doc.find_with_query('p')

@mojavelinux mojavelinux added this to the v2.x milestone Apr 6, 2019
mojavelinux added a commit to mojavelinux/asciidoctor that referenced this issue Nov 9, 2023
@mojavelinux
Copy link
Member Author

I really like the get_by idea. Furthermore, what we can do is still add support for the :single_result option to find_by. This option will be used internally by get_by. The get_by method will always return a block or nil, whereas find_by with the :single_result option will return an array with at most one item.

I don't think we need to throw an exception if more than one result is found. That just makes the method have to do more work than it needs to. The user can always find out if too many results are being returned by using find_by.

This is by no means a perfect API. We are trying to adapt it to existing code as best we can. In a future version, we could always consider mapping this functionality to new method names.

@mojavelinux mojavelinux modified the milestones: v2.x, v2.1.x Nov 9, 2023
@mojavelinux mojavelinux self-assigned this Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants
@dsisnero @mojavelinux @ggrossetie and others