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 :fn/index as alias for :index in map syntax #603

Closed
dgr opened this issue Jul 11, 2024 · 5 comments
Closed

Add :fn/index as alias for :index in map syntax #603

dgr opened this issue Jul 11, 2024 · 5 comments

Comments

@dgr
Copy link
Contributor

dgr commented Jul 11, 2024

The map syntax supports an :index key that operates similarly to the various :fn/* keywords.

Indeed, in impl/xpath.clj the clause multimethod has all the :fn/* keywords as method selectors as well as :index. This suggests that perhaps :fn/index should be added as an alias for :index and :index be deprecated.

If folks agree that this is the way to go, I can submit a PR.

@lread
Copy link
Collaborator

lread commented Jul 11, 2024

So nice to see you active and interested in Etaoin @dgr!

I had to refresh my brain on how this all works (@borkdude and I are not the original authors of Etaoin but volunteered to maintain the project under clj-commons). So the :fn/* is for map syntax queries.

I'm not sure why :index is not :fn/index. Maybe it did not feel like a function to the original author?

So, my question to you is, why do you want this change? Do you feel strongly about it?
Were you confused and thrown off by :index not being :fn/index?

@dgr
Copy link
Contributor Author

dgr commented Jul 11, 2024

I think it's just about consistency. It seems like the :fn/* keywords are there because other map keys other than :tag end up as attributes of the element that are searched for. I don't feel strongly about it one way or another if there is a good reason for it to be different. But again, deep in the code, it's handled exactly the same way when constructing xpath. For backward compatibility, I'd just add a duplicate method to the multimethod (with defmethods in imps/util.clj), that way existing code would continue to work.

If you look at the xpath, it's clear that :index is not handled the same way as other attributes.

etaoin.api> (query/expand driver {:tag :foo :attr "blah"})
["xpath" ".//foo[@attr=\"blah\"]"]
etaoin.api> (query/expand driver {:tag :foo :index 1})
["xpath" ".//foo[1]"]
etaoin.api> (query/expand driver {:tag :foo :fn/enabled true})
["xpath" ".//foo[@enabled=true()]"]
etaoin.api> (query/expand driver {:tag :foo :class "blah"})
["xpath" ".//foo[@class=\"blah\"]"]

@lread
Copy link
Collaborator

lread commented Jul 11, 2024

Thanks @dgr, adding a synonym sounds fine to me.

Probably no need to deprecate :index but docs could be updated to replace :index with :fn/index (and would not need to mention :index). Sound good?

@dgr
Copy link
Contributor Author

dgr commented Jul 12, 2024

That sounds fine to me. Do we need any more opinions before I create a PR, or should I just for it?

@lread
Copy link
Collaborator

lread commented Jul 12, 2024

Seems like a solid plan to me! Feel free to proceed with a PR!

@lread lread closed this as completed Jul 17, 2024
@lread lread reopened this Jul 17, 2024
@lread lread closed this as completed in 386918a Jul 17, 2024
lread added a commit that referenced this issue Jul 28, 2024
…ridriver-logs

* origin/master:
  Ignore emacs backup files (#609)
  Add :fn/index as alias for :index in map syntax (#603) (#608)
  doc: thank Dave Roberts for contribution [skip ci] (#607)
  Add support for shadow DOM #604 (#606)
  doc: user-guide: add list of :fn/* (#605)
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