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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using contains for queries which use labels to match #387

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

joelbyler
Copy link

I'm not sure if this is the perfect fix for #306 but I was able to get the new test to pass using this revised xpath. That thing is so hard to follow though, I'm sure I might have missed something. I'm opening this in the hopes to start a conversation about fixing this issue and I'm glad to help out. If this does actually fix the issue then 馃帀, but I'm betting there's more to it.

Also, what are your thoughts on making that xpaths easier to parse visually? I spent some time breaking bits of it apart locally but didn't commit any of that here. Perhaps I can offer some help there to lower the bar of entry for others, let me know.

Thanks for reviewing and thanks in advance for your feedback!

@joelbyler
Copy link
Author

... and there is a check that labels have a for attribute. Hmm... I'll look into that.

@keathley
Copy link
Member

@joelbyler Thanks for working on this! I'm sorry about the xpath stuff. It's gross. I originally intended on building a library to compose xpath queries but never made time for it. I haven't come up with a better solution yet but if you have thoughts I'd be interested in hearing them.

@fusillicode
Copy link

Hi guys 馃槃

is this still relevant? I would love to align this PR with the latest master if needed 馃槉

P.S: thanks a lot for this great library! 馃檱

@mhanberg mhanberg closed this Dec 4, 2020
@mhanberg mhanberg deleted the branch elixir-wallaby:main December 4, 2020 05:34
@mhanberg mhanberg reopened this Dec 4, 2020
@mhanberg mhanberg changed the base branch from master to main December 4, 2020 15:07
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

4 participants