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

Issue 29097: adjusted ids in entities and entitylist to be PascalCase #29117

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

Conversation

MrMothman
Copy link

@MrMothman MrMothman commented Jun 17, 2024

About the PR

I adjusted files inside of entities and entitylist so that the ids: feild is all in pascal case: the entities list is below
./Markers/clientsideclone.yml: id: clientsideclone
./Markers/construction_ghost.yml: id: constructionghost
./Markers/drag_shadow.yml: id: dragshadow
./Markers/hover_entity.yml: id: hoverentity
./Objects/Fun/dice.yml: id: d20Dice
./Objects/Fun/dice.yml: id: d12Dice
./Objects/Fun/dice.yml: id: d10Dice
./Objects/Fun/dice.yml: id: d8Dice
./Objects/Fun/dice.yml: id: d6Dice
./Objects/Fun/dice.yml: id: d4Dice
./Structures/hydro_tray.yml: id: hydroponicsTray
./Structures/Machines/Computers/computers.yml: id: computerBodyScanner
./Structures/soil.yml: id: hydroponicsSoil
./Structures/Storage/filing_cabinets.yml: id: filingCabinet
./Structures/Storage/filing_cabinets.yml: id: filingCabinetTall
./Structures/Storage/filing_cabinets.yml: id: filingCabinetDrawer
./Structures/Storage/filing_cabinets.yml: id: filingCabinetRandom
./Structures/Storage/filing_cabinets.yml: id: filingCabinetTallRandom
./Structures/Storage/filing_cabinets.yml: id: filingCabinetDrawerRandom
./Objects/Tools/t-ray.yml: id: trayScanner
Along with the above I also changed all occurrences of those ids: in files where they were called

Why / Balance

#28693
Also, I found this to be an easier task and want to get use to working within this project before I take on bigger projects.
Also, Cases matter to keep the same:
NOTE: I didn't changed dice_bag.yml or any behavior ids:, I'm not sure if we want to change those but if we do It is an easy fix.

Technical details

Media

  • [X ] I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

  • tweak: ids: fields to match PascalCase usage else where.

@github-actions github-actions bot added the No C# For things that don't need code. label Jun 17, 2024
Copy link
Contributor

@TheShuEd TheShuEd left a comment

Choose a reason for hiding this comment

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

you also need to change the names of all the relationships to the changed entities.

to give you an example:
https://github.com/search?q=repo%3Aspace-wizards%2Fspace-station-14%20clientsideclone&type=code

@TheShuEd TheShuEd added the Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. label Jun 17, 2024
Copy link
Contributor

@lzk228 lzk228 left a comment

Choose a reason for hiding this comment

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

also you need to put all changed entities' ids in /Resources/migration.yml

@deltanedas
Copy link
Contributor

this also doesnt account for the few ids with _ in them

@MrMothman
Copy link
Author

Thank you I assumed it was somewhere else as well @lzk228 @TheShuEd : ) @deltanedas all the ones with _ in the name are behavior Ids: and i didn't know if we also wanted those changed. I didn't do behavior ids: or the dice_bag.yml because those were pretty weird and i didn't know if their was a reason.

@lzk228
Copy link
Contributor

lzk228 commented Jun 17, 2024

ok leave it as it is for now, just do what i and Ed said

@github-actions github-actions bot added Status: Needs Review This PR requires new reviews before it can be merged. Changes: Map Can be reviewed or fixed by people who are knowledgeable with mapping. Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. and removed Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. Status: Needs Review This PR requires new reviews before it can be merged. labels Jun 17, 2024
@MrMothman
Copy link
Author

i ran find . -type f -exec grep -H -f p.txt {} + > FilesToAdjust.txt
at root with FilesToAdjust.txt containing all the names of ids: I've changed and had many occurrences of those so I changed all of those and the mirgration.yml actually didn't have any occurrences in it: )

@lzk228
Copy link
Contributor

lzk228 commented Jun 17, 2024

you dont need to change maps if you write all changes in migration.yml

@MrMothman
Copy link
Author

ahhh the more you learn then. I am sorry about that
today I learned

@TheShuEd
Copy link
Contributor

you still need to add changes to the migration because the forks have their maps still using those items. Forcing ALL projects to rewrite these changes manually is a very bad idea.

@MrMothman
Copy link
Author

MrMothman commented Jun 17, 2024

I got that sorry I also had to figure out the error that was caused when I changed the entityList .ymls. I am going to go track down where I accidentally changed the sprite load.

@MrMothman
Copy link
Author

MrMothman commented Jun 18, 2024

One day later and I got all the checks: )

@lzk228
Copy link
Contributor

lzk228 commented Jun 18, 2024

Ed tfym approved
@MrMothman you should revert all map changes

Comment on lines +370 to +375






Copy link
Contributor

Choose a reason for hiding this comment

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

remove those extra lines

@github-actions github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Jun 19, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: Map Can be reviewed or fixed by people who are knowledgeable with mapping. Merge Conflict This PR currently has conflicts that need to be addressed. No C# For things that don't need code. Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants