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

feat: save #294

Merged
merged 8 commits into from
Nov 3, 2024
Merged

Conversation

qudix
Copy link
Contributor

@qudix qudix commented Nov 2, 2024

  • I renamed TESForm::LookupByX to TESForm::GetFormByX to match CommonlibF4 and what Bethesda internally calls them

@ThirdEyeSqueegee
Copy link
Member

What's the point of the breaking change? There's really no need to rename it, doesn't matter what Bethesda calls it internally

@qudix
Copy link
Contributor Author

qudix commented Nov 2, 2024

I can remove it if it's not wanted, but I guess making a poll for it would be nice. I generally try to go for parity and "correctness" when possible, unless it can be improved beyond what Bethesda intended.

Copy link
Contributor

@shadeMe shadeMe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ThirdEyeSqueegee - The original name is semantically the same as the proposed one, i.e., it's correct. While "parity/correctness" is nice to have, this change will introduce significant breakage (especially given how often those lookup functions are likely to be used) in downstream plugins for no benefit. The constant raft of breaking changes to the library is already hard to keep up with (especially if one isn't updating their plugins regularly), so let's please just not.

Instead, if you feel that it's really necessary, let's mention the discrepancy as a comment.

@ThirdEyeSqueegee ThirdEyeSqueegee merged commit 05ead67 into Starfield-Reverse-Engineering:main Nov 3, 2024
4 of 5 checks passed
@qudix qudix deleted the dev/save branch November 4, 2024 03:46
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.

3 participants