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

Odyssey materials #597

Merged
merged 3 commits into from Jun 3, 2021
Merged

Conversation

Gimi1967
Copy link
Contributor

@Gimi1967 Gimi1967 commented Jun 3, 2021

Updated entryData.json file with Odyssey Material.
Source for data: INARA, ED Wiki, EDD.
FormattedName: Not sufficiently verified. Where I did not know, I have used truncated display name in lower case.
SettlementType, BuildingType, ContainerType data from ED Wiki.
My first attempt at a pull request. Scary

Updated entryData.json file for Odyssey Material. 
Source for data: INARA, ED Wiki, EDD.
FormattedName: Not sufficiently verified. Where I did not know, I have used truncated display name in lower case.
SettlementType, BuildingType, ContainerType data from ED Wiki.
@Gimi1967 Gimi1967 mentioned this pull request Jun 3, 2021
@msarilar
Copy link
Owner

msarilar commented Jun 3, 2021

great stuff!

I got a few remarks, I can do the changes myself if you prefer

"ValueCr": null,
"BarterCost": null,
"BarterValue": null,
Copy link
Owner

@msarilar msarilar Jun 3, 2021

Choose a reason for hiding this comment

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

all the null fields can be completely removed

{
   "Value": 10,
   "Something": null
}

is equivalent to

{
   "Value": 10
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a few cases the null value indicates that we are missing the value, as in we dont know it yet.
I will leave those inn, or should I put in a diffrent placeholder?

Copy link
Owner

Choose a reason for hiding this comment

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

you could put -1 on the ones you think are missing - it will be a reminder of incomplete data in the gui so it's not forgotten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the missing data is the ValueCr for some items.

Updated based on discussion.
@msarilar msarilar marked this pull request as ready for review June 3, 2021 21:30
Copy link
Owner

@msarilar msarilar left a comment

Choose a reason for hiding this comment

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

excellent work thanks a lot

@msarilar msarilar merged commit 018e9e1 into msarilar:master Jun 3, 2021
@Gimi1967
Copy link
Contributor Author

Gimi1967 commented Jun 3, 2021

Thank you. This last part with pull requests and stuff was fun.
Never got to change
"ValueCr": null
to
"ValueCr": -1

@Gimi1967 Gimi1967 changed the title Gimi1967 odyssey materials Odyssey materials Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants