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

feature: allow overriding module address #118

Merged
merged 12 commits into from
Jan 21, 2025

Conversation

Dhruv-Mishra
Copy link
Contributor

No description provided.

src/constants.ts Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
src/constants.ts Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
@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