Skip to content

Separate Small and Large Oil Rig notification settings #339

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eddiewastaken
Copy link

@eddiewastaken eddiewastaken commented Jun 22, 2024

This allows independently setting Small and Large Oil Rig notifications for Heavy Scientists and Locked Crate unlocks.

@eddiewastaken
Copy link
Author

Due to the removal of the existing generic Oil Rig notification settings to make them more specific, this PR, as it exists causes an issue when the user updates their existing installation, as they will have a notification setting key in their instance file which no longer exists, and an exception will be thrown. I've resolved this issue (on a separate branch) with commit 1b83863 which I'm happy to merge into this PR, but I’ve left it out for now in case the PR reviewer dislikes that approach 😁

@eddiewastaken
Copy link
Author

(Fixed a couple of missed translations with a force push to keep the PR as one commit)

@FaiThiX
Copy link
Contributor

FaiThiX commented Jun 27, 2024

I like that change but one thing you have to change is the lang files. we only translate into english and all other languages are updated by crowdin. so you have to remove that from your commits

@eddiewastaken
Copy link
Author

All done, thanks for letting me know!

Due to the removal of the existing generic Oil Rig notification settings to make them more specific, this PR, as it exists causes an issue when the user updates their existing installation, as they will have a notification setting key in their instance file which no longer exists, and an exception will be thrown. I've resolved this issue (on a separate branch) with commit 1b83863 which I'm happy to merge into this PR, but I’ve left it out for now in case the PR reviewer dislikes that approach 😁

Do you have any thoughts on this? Should I add this change to the PR, or do you have another idea/approach?

@eddiewastaken
Copy link
Author

eddiewastaken commented Apr 12, 2025

Just a note to maintainers - it's generally discouraging to contributers when (useful, non-conflicting and CI passing) PRs are left stagnant/ignored for many months, while more recent PRs are being merged: #382, #392, #418, #419..

@alexemanuelol
Copy link
Owner

Just a note to maintainers - it's generally discouraging to contributers when (useful, non-conflicting and CI passing) PRs are left stagnant/ignored for many months, while more recent PRs are being merged: #382, #392, #418, #419..

You might know (or not) that I've been away from the project for a while. I've recently come back to finish what I started last year, migrating everything to TypeScript. While working on that, I've intentionally held off on adding new features since most of it would need to be rewritten for the TS version anyway.

The few PRs I've merged lately have either been quick fixes or widely requested features that line up with Facepunch's recent updates to Rust+.

Once the TypeScript migration is solid, I'll start going through all the issues, bug reports, suggestions, and PRs.

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