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

lint: Validate strings #234

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

german77
Copy link
Contributor

@german77 german77 commented Jan 9, 2025

Is not invalid string, is invalid swing. This PR adds an additional check to check-format.py. It finds strings in the source code and checks if the string is defined in the nso.

This check can't validate if the string is the correct one for that specific line of code but at least ensures typos or incomplete strings can't be added to the project.

It also adds a new tool generate-strings.py that creates the csv file from the main.nso file.


This change is Reviewable

Copy link
Owner

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 6 of 7 files reviewed, 11 unresolved discussions (waiting on @german77)


tools/generate-strings.py line 1 at r1 (raw file):

#!/ usr / bin / env python3

Suggestion:

#!/usr/bin/env python3

tools/generate-strings.py line 9 at r1 (raw file):

MIN_TEXT_SIZE = 0x2
MAX_TEXT_SIZE = 0x400

There are some more longer strings

Suggestion:

MAX_TEXT_SIZE = 0x600

tools/generate-strings.py line 20 at r1 (raw file):

        return
    if len(buffer) > MAX_TEXT_SIZE:
        return

Throw error here instead


tools/generate-strings.py line 31 at r1 (raw file):

        text = text.replace("\r", "\\r")
        text = text.replace("\t", "\\t")
        text = text.replace("\"", "\\\"")

Also add a replacement for backslash \ => \


tools/generate-strings.py line 44 at r1 (raw file):

def parse_nso(csv_file, nso_file):
    offset = START_OFFSET - NSO_OFFSET + 0x100

Why is that juggling with 0x100 done? I see that it fails to start at the right offset otherwise, but ... why?


tools/generate-strings.py line 46 at r1 (raw file):

    offset = START_OFFSET - NSO_OFFSET + 0x100
    end = END_OFFSET - NSO_OFFSET + 0x100
    nso_file.seek(offset);

Suggestion:

nso_file.seek(offset)

tools/generate-strings.py line 77 at r1 (raw file):

    
    nso_file.close()
    csv_file.close()

with to avoid closing

Code quote:

    csv_file = open(string_path, "w")
    nso_file = open(nso_path, "rb")

    parse_nso(csv_file, nso_file)

    nso_file.close()
    csv_file.close()

tools/generate-strings.py line 85 at r1 (raw file):

        print("main.nso not found")
        return
    create_string_table(project_root / 'data'/ "data_strings.csv", project_root / 'data'/ "main.nso");

Suggestion:

create_string_table(project_root / 'data'/ "data_strings.csv", project_root / 'data'/ "main.nso")

src/Player/PlayerConst.cpp line 0 at r1 (raw file):
Please also rename the PlayerConst members for all typos fixed in here


tools/check-format.py line 447 at r1 (raw file):

def read_csv_file(path):
    if not os.path.isfile(path):
        return []

raise exception instead

Code quote:

return []

tools/check-format.py line 456 at r1 (raw file):

        rows.append(row)

    file.close()

with to avoid manually closing the file

Code quote:

    file = open(path, mode="r")
    reader = csv.reader(file)

    rows = []
    for row in reader:
        rows.append(row)

    file.close()

tools/check-format.py line 475 at r1 (raw file):

                file_path = os.path.join(root, file)
                file_str = str(file_path)
                check_file(file_str, string_table)

Do not add parameters for individual checks here - instead, create a function get_string_table() and use the @cache tool to avoid performance hits.

Copy link
Contributor Author

@german77 german77 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 8 files reviewed, 11 unresolved discussions (waiting on @MonsterDruide1)


src/Player/PlayerConst.cpp line at r1 (raw file):

Previously, MonsterDruide1 wrote…

Please also rename the PlayerConst members for all typos fixed in here

Done.


tools/check-format.py line 447 at r1 (raw file):

Previously, MonsterDruide1 wrote…

raise exception instead

Done.


tools/check-format.py line 475 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Do not add parameters for individual checks here - instead, create a function get_string_table() and use the @cache tool to avoid performance hits.

Done.


tools/generate-strings.py line 9 at r1 (raw file):

Previously, MonsterDruide1 wrote…

There are some more longer strings

Done.


tools/generate-strings.py line 20 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Throw error here instead

Done.


tools/generate-strings.py line 31 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Also add a replacement for backslash \ => \

Done.


tools/generate-strings.py line 44 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Why is that juggling with 0x100 done? I see that it fails to start at the right offset otherwise, but ... why?

Done. Not sure seems like the nso has a header


tools/generate-strings.py line 77 at r1 (raw file):

Previously, MonsterDruide1 wrote…

with to avoid closing

Done.


tools/generate-strings.py line 1 at r1 (raw file):

#!/ usr / bin / env python3

Done.


tools/generate-strings.py line 46 at r1 (raw file):

    offset = START_OFFSET - NSO_OFFSET + 0x100
    end = END_OFFSET - NSO_OFFSET + 0x100
    nso_file.seek(offset);

Done.


tools/generate-strings.py line 85 at r1 (raw file):

        print("main.nso not found")
        return
    create_string_table(project_root / 'data'/ "data_strings.csv", project_root / 'data'/ "main.nso");

Done.

Copy link
Owner

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @german77)

@MonsterDruide1 MonsterDruide1 merged commit bc0ca21 into MonsterDruide1:master Jan 13, 2025
6 checks passed
@german77 german77 deleted the validateStrings branch January 13, 2025 18:45
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.

2 participants