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 filtering to ._printChanges() #2955

Closed
wants to merge 1 commit into from

Conversation

DandyLyons
Copy link
Contributor

This PR adds to static methods to _ReducerPrinter to allow the user to easily filter _printChanges to only print the changes that they are concerned with. The user can filter by actions (the action and change will print) or by changes (the user specifies which changes they are concerned with and the printer will print the change and the action that caused it).

@mbrandonw
Copy link
Member

Hi @DandyLyons, thanks for taking the time to explore this. There is an alternative API design that might be worth exploring. You could have the method take a variadic list of PartialCaseKeyPath<Action>, which would allow you to specify any number of sub-actions in a very succinct syntax:

Feature()
  ._printChanges(
    actions: \.destination.chidl1, \.destination.child2
  )

However, I will also say that it seems this tool doesn't need access to the internals of the library to be implemented. This means people are free to add this tool to their own projects or even open source it if they find it handy.

We will think a bit more about adding this to TCA proper, or whether we think it would be better to be maintained externally.

@DandyLyons
Copy link
Contributor Author

That would be a much nicer syntax. I don't know PartialCaseKeyPath well enough to know how to implement that.

I'm also not sure how/if PartialCaseKeyPath would work with the filtered(changes:) use case. Changes aren't known statically, so I don't suppose there is a way to derive a PartialCaseKeyPath from a series of changes.

Yes, please let me know if this is a better fit outside of TCA proper. I would love to open source it, if not. Personally _printChanges is one of my favorite features of TCA but it becomes far less useful when it becomes a deluge of print statements. So I feel like it would be a fairly core feature.

Eventually I would like to implement a _logChanges() that uses the same infrastructure but does an OSLog instead of a print statement. As an OSLog, we could "Jump to Line" from the console. And we could automatically derive the subsystem for the Log object based of off the Reducer name.

@stephencelis
Copy link
Member

@DandyLyons Thanks again for exploring this! And I think that _logChanges would also be great to explore soon.

For now, since this all still seems more like active experimentation/discussion territory, I'm going to close out the PR, which I don't think we're ready to merge. But in the future, let's start GitHub discussions to share potential additions to these APIs, and if it makes sense they can graduate to PRs for merging.

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

3 participants