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

Issues 671 and 672 #673

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Issues 671 and 672 #673

wants to merge 5 commits into from

Conversation

dgr
Copy link
Contributor

@dgr dgr commented Sep 23, 2024

Closes #671
Closes #672

Please complete and include the following checklist:

  • I have read CONTRIBUTING and the Etaoin Developer Guide.

  • This PR corresponds to an issue that the Etaoin maintainers have agreed to address. Not yet. I waited for a couple of days after discussing both Support sequentials in addition to vectors for query #671 and query doesn't correctly handle vectors inside of vectors #672 last week. I started to code up what I agreed to (throw better errors), but because of the way that query is coded, it rapidly became a bit of a mess. I stepped back and asked myself, "Is this the Right Answer?" I didn't feel like it. So, I left that alone and coded up what I had proposed. The code fell into place very naturally and so I kept going, adding regression tests, updating the User Guide, and ultimately the CHANGELOG. So, here it is. In this PR, query supports arbitrary sequentials, not just vectors. This means vectors, lists, seqs of all sorts, etc. Also, it supports an arbitrary hierarchy of sequentials which are then flatten-ed before the traversal starts. This allows for easy storage of partial paths (e.g., path prefixes) in variables which can simply be added to a sequential query as well as generation of parts of the path programmatically when the result of that generation is something like a lazy-seq (e.g., something returned by all the lazy collection routines).

  • This PR contains test(s) to protect against future regressions

  • I have updated CHANGELOG.adoc with a description of the addressed issue.

@lread
Copy link
Collaborator

lread commented Sep 24, 2024

So Dave, we agreed we weren't solving any known pain points for Etaoin users with #671, but you went ahead and implemented anyway?

@lread
Copy link
Collaborator

lread commented Sep 24, 2024

Aside: I can't get failing tests to pass, probably not related to your change but feels like maybe #674 and/or #646.

@dgr
Copy link
Contributor Author

dgr commented Sep 24, 2024

Yea, I implemented it anyway. If you want to reject the PR, that's your call. But I think this is the right thing to do. I know that you see it as adding functionality. I see it as solving a bunch of cases that were handled poorly in a way that's elegant and enhances usability. But you're the maintainer, so it's your call.

@lread
Copy link
Collaborator

lread commented Sep 24, 2024

Hi Dave, First, thanks for your continued interest and contributions to Etaoin. Moving forward, though, it is important that we have clear agreement before submitting a PR.

Submitting a PR without agreement creates unexpected work for me and puts me in a tough position - either having to disappoint the submitter by rejecting their PR or spending time re-reviewing their proposal and now also their code.

While all PRs are certainly well-intentioned, remember that as the maintainer, I need to understand and maintain all changes long after contributors have moved on to other things.

@dgr
Copy link
Contributor Author

dgr commented Sep 24, 2024

Yep, I get it. Do what you think is right. I won't be offended either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants