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

Introduce Scarlet / Violet new items #787

Merged
merged 7 commits into from
Jan 14, 2023
Merged

Conversation

giginet
Copy link
Contributor

@giginet giginet commented Dec 5, 2022

I added a new item since Scarlet / Violet.

All data are migrated from here.
PMSV/Item Names.txt at main · Ruimusume/PMSV

I corrected some Japanese and English names.

@@ -1606,5 +1606,448 @@ id,identifier,category_id,cost,fling_power,fling_effect_id
1656,carrot-seeds,22,20,,
1657,ability-patch,26,20,,
1658,reins-of-unity,22,0,,
1659,adamant-crystal,,,,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What number of ID should I start from?

Some items are already available since Legends Arceus.

Copy link
Member

Choose a reason for hiding this comment

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

I think the numbering should be consequent, so it's fine the one used is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean, the current numbering doesn't have any problem?

Copy link
Member

Choose a reason for hiding this comment

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

Hi, yes your numbering is good. Anyways, I'm not sure if we can leave empty the category_id, cost,fling_power and fling_effect_id. Don't you have at least the category_id for your new data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no original data for category_id so I have to fill this column by hand.

Can we add new item_categories like tm_materials or sandwich_ingredients?

Copy link
Member

Choose a reason for hiding this comment

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

we can add new categories if needed, yes

@Naramsim
Copy link
Member

Naramsim commented Dec 5, 2022

hi @giginet , thanks a lot for your effort!

Could you please take a look at this: #783 (comment)

@giginet
Copy link
Contributor Author

giginet commented Dec 9, 2022

I'll add category_ids. However, this column is not necessary for my use case.
So I'll update this PR, but it will be a little late.

@giginet giginet force-pushed the sv-items branch 2 times, most recently from 25d2b45 to 7c7b199 Compare January 3, 2023 08:07
@giginet
Copy link
Contributor Author

giginet commented Jan 3, 2023

@Naramsim

Hi. I added new item_categories and filled all item_category columns.
Could you review?

@giginet giginet requested a review from Naramsim January 6, 2023 06:16
1674,big-bamboo-shoot,24,1500,,
1675,scroll-of-darkness,21,0,,
1676,scroll-of-waters,21,0,,
1677,,,,,
Copy link
Member

Choose a reason for hiding this comment

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

Is it normal that is all empty?

Copy link
Contributor Author

@giginet giginet Jan 8, 2023

Choose a reason for hiding this comment

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

I migrated from the original data. It seems to be missing numbers. These columns should be removed.

I'll remove these columns and fill the following IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Naramsim I removed missing columns and filled the following IDs.

ba8ff8f

@Naramsim
Copy link
Member

Naramsim commented Jan 7, 2023

Thanks for the PR, before merging I'd like to have a buildable master, so I'd prefer to wait for this: #807 (comment)

@Naramsim
Copy link
Member

Naramsim commented Jan 9, 2023

Hi @giginet , would it be difficult for you to provide us with the cost of the items?

@giginet
Copy link
Contributor Author

giginet commented Jan 10, 2023

I don't have any canonical data about costs.
So I have to fill them by hand from Wiki pages.

The cost column is necessary for merging this?
I'd like to merge this first and add costs after merging.

@Naramsim
Copy link
Member

For us, it would be better to have the cost since right now it's a mandatory field. I can turn it to a nullable field though.

@giginet
Copy link
Contributor Author

giginet commented Jan 11, 2023

OK. I'll try to fill in all the cost columns in my spare time.

Could you review other PRs first?

@Naramsim
Copy link
Member

I'll check out some other PRs :)

@giginet
Copy link
Contributor Author

giginet commented Jan 14, 2023

@Naramsim Hi. I filled all cost columns as far as I know.
bb56e09

Could you review this?

@Naramsim Naramsim changed the base branch from master to staging January 14, 2023 20:44
@Naramsim Naramsim merged commit 61c5154 into PokeAPI:staging Jan 14, 2023
@Naramsim
Copy link
Member

Hi @giginet , there's a problem. The ids in items.csv and item_names.csv differ.

For example white-dish is 2098 and 2100

@giginet giginet deleted the sv-items branch January 15, 2023 05:27
@giginet giginet mentioned this pull request Jan 15, 2023
@giginet
Copy link
Contributor Author

giginet commented Jan 15, 2023

@Naramsim Sorry for not checking enough.
I fixed on this PR.
#819

@Naramsim
Copy link
Member

@Naramsim Sorry for not checking enough.
I fixed on this PR.
#819

All good! I tested everything locally and it builds fine!

All your PR are now merged! Thanks so much for the efforts! I'll add your name on our website as a contributor!

Now we can update the ids

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