Skip to content

Conversation

@Dhruv-Mishra
Copy link
Contributor

No description provided.

@kopy-kat
Copy link
Member

I think this approach looks good

@Dhruv-Mishra Dhruv-Mishra marked this pull request as ready for review January 15, 2025 17:41
@Dhruv-Mishra Dhruv-Mishra requested a review from kopy-kat January 15, 2025 17:41
@Dhruv-Mishra
Copy link
Contributor Author

@kopy-kat requesting review

@kopy-kat
Copy link
Member

nice - I think this looks good

imo it would be useful to write a sanity test check to make sure when you override the constants that actually the overwritten constants are used (eg on a module installation function) - imo its overkill to do this for all functions but having a sanity check would be good

@Dhruv-Mishra
Copy link
Contributor Author

nice - I think this looks good

imo it would be useful to write a sanity test check to make sure when you override the constants that actually the overwritten constants are used (eg on a module installation function) - imo its overkill to do this for all functions but having a sanity check would be good

Sounds good, I have added some unit tests in overrideConstants.test.ts file

@kopy-kat
Copy link
Member

great, this looks good to me - could you just please fix the merge issues and then this is good to go

@Dhruv-Mishra
Copy link
Contributor Author

great, this looks good to me - could you just please fix the merge issues and then this is good to go

Thanks
I have resolved the merge conflicts as well now

@kopy-kat kopy-kat merged commit 7bc8c43 into rhinestonewtf:main Jan 21, 2025
1 check failed
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.

2 participants