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.

Sign up for GitHub

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

Jump to bottom

cleanup instruments yml #32262

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

cleanup instruments yml #32262

wants to merge 3 commits into from
+129 −273

Conversation

lzk228
Copy link
Contributor

@lzk228 lzk228 commented Sep 17, 2024 •
edited
Loading

-144 LINES LESSGO

About the PR

  • move keyed in new file
  • make base instruments for every type
  • cleanup a lot of inherited things
  • changelog

Why / Balance

i love cleaning ymls

Technical details

before and after
before
after

Media

rsi path can be written only in SpriteComponent
Screenshot_20240918_001948

Requirements

Breaking changes

Changelog

🆑

  • tweak: Seashell is small size. Kalimba is percussion instrument.

@github-actions github-actions bot added the Changes: No C# Changes: Requires no C# knowledge to review or fix this item. label Sep 17, 2024
@lzk228
Copy link
Contributor Author

lzk228 commented Sep 17, 2024

yes, test fail IS related, but the best way to fix it is #31073

@PopGamer45
Copy link
Contributor

PopGamer45 commented Sep 20, 2024 •
edited
Loading

yes, test fail IS related, but the best way to fix it is #31073

Did you accidentally link the wrong PR by any chance maybe?

@lzk228
Copy link
Contributor Author

lzk228 commented Sep 20, 2024

yes, test fail IS related, but the best way to fix it is #31073

Did you accidentally link the wrong PR by any chance maybe?

no, test fails because i can buy a single crate for 2500 and use it in bounty for 15000
the pr i linked nerfs most bounty rewards

@SlamBamActionman SlamBamActionman added the S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. label Nov 14, 2024
@beck-thompson beck-thompson added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. T: Cleanup Type: Code clean-up, without being a full refactor or feature D3: Low Difficulty: Some codebase knowledge required. S: Needs Review Status: Requires additional reviews before being fully accepted size/L Denotes a PR that changes 1000-4999 lines. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 21, 2024
@lzk228 lzk228 mentioned this pull request Feb 18, 2025
Open
2 tasks
SlamBamActionman
SlamBamActionman approved these changes Feb 23, 2025
View reviewed changes
Copy link
Member

@SlamBamActionman SlamBamActionman left a comment

Choose a reason for hiding this comment

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

I don't think the cargo bounty thing should be a blocker for this PR; it should be fixed, but this kinda cleanup is important too.

@SlamBamActionman SlamBamActionman added the S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. label Feb 23, 2025
@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. and removed size/L Denotes a PR that changes 1000-4999 lines. labels Feb 23, 2025
slarticodefast
slarticodefast requested changes Feb 25, 2025
View reviewed changes
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

The cargo bounty problem is a blocker because it makes the intagration tests fail.

@lzk228
Copy link
Contributor Author

lzk228 commented Feb 25, 2025

so how should i resolve it?

@slarticodefast
Copy link
Member

so how should i resolve it?

The other PR has to be merged first I guess.
I just wanted to make sure no one merges this before the integration tests are working (since Slam said cargo bounties should not be a blocker).

@slarticodefast slarticodefast added the S: Requires Content PR Status: Requires a change to SS14, for which there is no open PR currently. label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slarticodefast slarticodefast slarticodefast requested changes

@SlamBamActionman SlamBamActionman SlamBamActionman approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Changes: No C# Changes: Requires no C# knowledge to review or fix this item. D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Review Status: Requires additional reviews before being fully accepted S: Requires Content PR Status: Requires a change to SS14, for which there is no open PR currently. size/M Denotes a PR that changes 100-999 lines. T: Cleanup Type: Code clean-up, without being a full refactor or feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lzk228 @PopGamer45 @slarticodefast @SlamBamActionman @beck-thompson