Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

How about contributing this as a PR to the DataFixerUpper repo? #4

Open
billybong opened this issue Apr 12, 2022 · 2 comments
Open

Comments

@billybong
Copy link

Hi @jellysquid3!
I tried this out yesterday and there are some really good stuff in your fork.
At a glance it seems to cut the rule optimization execution times by more than 50%, so I'd be very interested to seeing this added to the official repo. Given that it's open sourced there should be nothing stopping us from treating it as a PR like any other.

If you don't feel like spending time on it I could incorporate some of the ideas into the main repo if you don't mind, but I'd rather let you get the credit for the hard work.

@jellysquid3
Copy link
Owner

jellysquid3 commented Apr 12, 2022

Hi.

A lot of the reason for not making a PR was due to the changes being rather opinionated, and me being unable to really validate that they worked correctly. But people have been using the patches in Cadmium for some time now and I haven't heard complaints. It would be good to get some of the easy ones implemented upstream, as DFU seems to be a major pain point in the game still.

I can open PRs on the upstream repository for some of the easier improvements later this week. My patches will probably need updating, though, as this repository hasn't been touched in a few years... but I haven't looked yet.

@billybong
Copy link
Author

That would be awesome, thank you so much!
I replicated most of them locally on mainline and they all seemed to have fairly nice improvements when combined.
The only one I think could lead to discussions is the removal of Optional which depends on which camp you're in.
Personally I'm in the nullable annotation camp when it comes to perf related code, but there could be other opinions on the team.
But regardless it would be a shame to miss out on it since it had quite a dramatic effect.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants