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

turn bare materials into objects #47015

Closed
wants to merge 5 commits into from
Closed

turn bare materials into objects #47015

wants to merge 5 commits into from

Conversation

TheGoatGod
Copy link
Contributor

@TheGoatGod TheGoatGod commented Jan 25, 2021

Summary

Infrastructure "JSON: turn materials represented by a string into objects"

Purpose of change

some materials are still bare this script fixes them

Describe the solution

run the script

Describe alternatives you've considered

leave as is, currently still works

Testing

none needed as this is seen all over.

@TheGoatGod TheGoatGod changed the title Update bare materials turn bare materials into objects Jan 25, 2021
@BrettDong BrettDong added [JSON] Changes (can be) made in JSON [Python] Code made in Python Code: Tooling Tooling that is not part of the main game but is part of the repo. labels Jan 25, 2021
@int-ua
Copy link
Contributor

int-ua commented Jan 25, 2021

Do you plan on trying to merge scripts with #46964 and #47015 ?

@TheGoatGod
Copy link
Contributor Author

Do you plan on trying to merge scripts with #46964 and #47015 ?

how do you mean merge? if you mean 1 py no

@int-ua
Copy link
Contributor

int-ua commented Jan 25, 2021

Do you plan on trying to merge scripts with #46964 and #47015 ?

how do you mean merge? if you mean 1 py no

Ok, then can you please move these scripts into a separate PR? I think they should be reviewed together and not with the JSON data changes. It would've been fine if it was only one script but since you are adding scripts that are just a slightly different copies of each other I believe they should be grouped in one PR.

@actual-nh
Copy link
Contributor

Ok, then can you please move these scripts into a separate PR? I think they should be reviewed together and not with the JSON data changes. It would've been fine if it was only one script but since you are adding scripts that are just a slightly different copies of each other I believe they should be grouped in one PR.

It would be easier to review the JSON data changes if the likely-final version of the script had already been run (no worries about getting overwritten). For items, I've already spotted at least a few that could use "str_sp" instead of "str", for instance.

@TheGoatGod
Copy link
Contributor Author

TheGoatGod commented Jan 29, 2021

Do you plan on trying to merge scripts with #46964 and #47015 ?

how do you mean merge? if you mean 1 py no

Ok, then can you please move these scripts into a separate PR? I think they should be reviewed together and not with the JSON data changes. It would've been fine if it was only one script but since you are adding scripts that are just a slightly different copies of each other I believe they should be grouped in one PR.

yeah I don't mind them being different, they do different things, only part you really have to look at is below, but if you only look at this part, and compare. they are not the same.

# We only want JsonObjects
            if type(jo) is str:
                continue
            if type(jo.get('material')) == str:
                material = jo['material']
                jo['material'] = [material]
                change = True
            if type(jo.get('ammo')) == str and jo.get('type') == 'ammo':
                ammo = jo['ammo']
                jo['ammo'] = [ammo]
                change = True

@TheGoatGod
Copy link
Contributor Author

Ok, then can you please move these scripts into a separate PR? I think they should be reviewed together and not with the JSON data changes. It would've been fine if it was only one script but since you are adding scripts that are just a slightly different copies of each other I believe they should be grouped in one PR.

It would be easier to review the JSON data changes if the likely-final version of the script had already been run (no worries about getting overwritten). For items, I've already spotted at least a few that could use "str_sp" instead of "str", for instance.

i think this is a reply to my bare name pr #46964

@TheGoatGod
Copy link
Contributor Author

TheGoatGod commented Jan 29, 2021

the py here need edited/updated to actually do only ammo but I don't think it matters
i cant make the pr validator happy

@int-ua
Copy link
Contributor

int-ua commented Jan 29, 2021

The summary section should be either

#### Summary
Infrastructure "JSON: turn materials represented by a string into objects"

or

#### Summary
None

(as it may not be a change worth mentioning to users, not sure). My PR that removed the requirement of the SUMMARY: in the summary line was merged, it is now detected just by the section title.

@TheGoatGod
Copy link
Contributor Author

don't believe that its on my end why continuous-integration/travis-ci/pr is failing

@actual-nh
Copy link
Contributor

It isn't; see #47022.

@TheGoatGod TheGoatGod closed this Feb 1, 2021
@TheGoatGod TheGoatGod deleted the update-bare-materials branch February 1, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code: Tooling Tooling that is not part of the main game but is part of the repo. [JSON] Changes (can be) made in JSON [Python] Code made in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants