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

Rename all non-snake_case types. #27268

Conversation

warriorstar-orion
Copy link
Contributor

@warriorstar-orion warriorstar-orion commented Nov 2, 2024

What Does This PR Do

This PR converts all non-snake_case type names to an appropriate alternative. It provides a commensurate UpdatePaths script and applies it. It provides a SQL script for the production feedback table. It adds a check to check_grep2 to prohibit such naming in the future. It adds a schema migration Python script for updating the feedback table (doing player and json_datum_saves now in a sec)

This refactoring was performed with the following procedure:

Apply and store a renaming heuristic to offending type names

This was done with the script make_snake_case_json.py. The result was the JSON file snake_case_type_remap.json. Each element in the JSON file was a dictionary with "original" being the original path, and "replace" being the converted path.

Clean up sub-optimal renames

Some of these replacements weren't great so I manually added an "override" key to those renames, so they wouldn't get clobbered by running the make_snake_case.json script again. For example:

    // ...
	{
		"original": "/obj/item/clothing/head/HoS",
		"replace": "/obj/item/clothing/head/ho_s",
		"override": "/obj/item/clothing/head/hos"
	},
    // ...

Generate the UpdatePaths script from the renames

generate_snake_case_updatepaths.py is a straightforward script which generated the UpdatePaths script. The results were spot-checked for correctness.

Perform a search and replace on code

perform_snake_case_search_replace.py was used to perform the actual search and replace on the codebase, ensuring that parameter names and the like were all properly affected. The results were spot-checked and it was ensured the code compiled.

Use the JSON file as input for a SQL migration.

The new schema migration script 60-61.py runs over all feedback rows and updates them based on the contents of the JSON renaming information. It is pretty fast:

PS D:\ExternalRepos\third_party\Paradise> python .\SQL\updates\60-61.py localhost para_migrations goaway parastats_prod_snake_case
[2024-11-02 23:23:40.101977] Connected to parastats_prod_snake_case
[2024-11-02 23:23:40.102977] Running query
[2024-11-02 23:23:40.103978] Fetching all data
[2024-11-02 23:23:41.318861] Processing 681114 rows...
[2024-11-02 23:23:59.895230] Running 2134 SQL updates
[2024-11-02 23:24:00.104232] Committing data
[2024-11-02 23:24:00.111231] Script done

Why It's Good For The Game

ORDNUNG MUSS SEIN Consistency in naming schemes is good because 1: it makes the code style more consistent, meaning less cognitive load for contributors, 2: it means not having to worry/think about case-sensitive regexes/searches looking for types, and 3: it means admins don't have to bang their heads against finding the right name for objects in the game menu.

Testing

See above. The scripts and JSON file can be checked for correctness, and the results of the entire process recreated if necessary.


Declaration

  • I confirm that I either do not require pre-approval for this PR, or I have obtained such approval and have included a screenshot to demonstrate this below.

Changelog

NPFC

@AffectedArc07
Copy link
Member

@warriorstar-orion

I will try and keep the SQL script updated as time goes on but there will inevitably be a lag between what's in prod and what rows the SQL script touches. It should be straightforward to run the SQL generating script on the handful of rounds that were missed between submission and database change.

If before merge we halt new merges I can run the script and give you the repo output. Or you can make it like the other SQL update files that end in .py and have it apply as a migration in that fashion.

@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally SQL Change This PR modifies the game database. This PR must go through the host. Map Edit This PR will modify a map Configuration Change This PR changes the game configuration files. Please run via the host. labels Nov 2, 2024
SQL/updates/60-61.py Outdated Show resolved Hide resolved
@AffectedArc07
Copy link
Member

To make CI work, append tools/ci/generate_sql_scripts.py

# And write the files and tell them to be used
for file in orderedSqlFiles:
    if file.endswith(".py"):
        # Begin snowflakery
        if file == "16-17.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb feedback round\n")
        elif file == "17-18.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb feedback feedback_2\n")
        elif file == "31-32.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb\n")
        elif file == "38-39.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb\n")
        else:
            print("ERROR: CI failed due to invalid python file in SQL/updates")
            exit(1)

And dont kill me for my python use, I wrote this script 4 years ago.

@warriorstar-orion
Copy link
Contributor Author

Will take a look at the players and saved_json_datums tables tomorrow.

@warriorstar-orion
Copy link
Contributor Author

To make CI work, append tools/ci/generate_sql_scripts.py

# And write the files and tell them to be used
for file in orderedSqlFiles:
    if file.endswith(".py"):
        # Begin snowflakery
        if file == "16-17.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb feedback round\n")
        elif file == "17-18.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb feedback feedback_2\n")
        elif file == "31-32.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb\n")
        elif file == "38-39.py":
            scriptLines.append("python3 SQL/updates/" + str(file) + " 127.0.0.1 root root paradise_gamedb\n")
        else:
            print("ERROR: CI failed due to invalid python file in SQL/updates")
            exit(1)

And dont kill me for my python use, I wrote this script 4 years ago.

Done.

@1080pCat 1080pCat added Code Improvement This PR will improve the code quality of the codebase Large PR This PR will lag you if you try to review it labels Nov 4, 2024
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting review This PR is awaiting review from the review team and removed -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally labels Nov 5, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Nov 6, 2024
@warriorstar-orion warriorstar-orion requested a review from a team as a code owner November 14, 2024 02:26
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Nov 14, 2024
Copy link
Member

@Burzah Burzah left a comment

Choose a reason for hiding this comment

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

Another banger.
SQL properly updated to next version.
An additional maintainer should look over this when able.

@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting merge This PR is ready for merge and removed -Status: Awaiting review This PR is awaiting review from the review team labels Nov 14, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Nov 21, 2024
@warriorstar-orion
Copy link
Contributor Author

@AffectedArc07 I added stanzas to the migration to update the characters.gear column and json_datum_saves.slotjson column. Any other important JSON columns I'm missing?

@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Nov 22, 2024
Copy link
Member

@Burzah Burzah left a comment

Choose a reason for hiding this comment

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

SQL version bump!
Make sure to change the script from 61-62.py to 62-63.py

config/example/config.toml Outdated Show resolved Hide resolved
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting review This PR is awaiting review from the review team and removed -Status: Awaiting merge This PR is ready for merge labels Nov 24, 2024
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting merge This PR is ready for merge and removed -Status: Awaiting review This PR is awaiting review from the review team labels Nov 24, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Nov 24, 2024
@Burzah Burzah removed the Merge Conflict This PR is merge conflicted label Nov 25, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Nov 25, 2024
@Burzah Burzah removed the Merge Conflict This PR is merge conflicted label Nov 26, 2024
@warriorstar-orion warriorstar-orion changed the title refactor: Rename all non-snake_case types (not procs or vars (yet)). Rename all non-snake_case types. Nov 30, 2024
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Nov 30, 2024
@AffectedArc07 AffectedArc07 added this pull request to the merge queue Nov 30, 2024
Merged via the queue into ParadiseSS13:master with commit 0ffa830 Nov 30, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Status: Awaiting merge This PR is ready for merge Code Improvement This PR will improve the code quality of the codebase Configuration Change This PR changes the game configuration files. Please run via the host. Large PR This PR will lag you if you try to review it Map Edit This PR will modify a map Merge Conflict This PR is merge conflicted SQL Change This PR modifies the game database. This PR must go through the host.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants