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

util: update gen_data scripts to use xivapi library #28

Merged
merged 10 commits into from
Jan 4, 2024
Merged

util: update gen_data scripts to use xivapi library #28

merged 10 commits into from
Jan 4, 2024

Conversation

wexxlee
Copy link
Collaborator

@wexxlee wexxlee commented Jan 1, 2024

Util cleanup project, part 2 of 3 (ref: #26)

This updates the remaining util/gen_(data) scripts to use the new xivapi library. I've deleted coinach.ts, since we are no longer reading SaintCoinach data directly and the file-writer function is now in the xivapi library.

All 3 scripts were run and updated output files are included. The changes in effect_id.ts and pet_names.ts are routine; the underlying game data has updated since the last time these scripts were run. (These same changes occur if you run the unmodified scripts.)

The world_ids.ts file has substantial changes. The original xivapi call in that script did not include a limit= param in the url; as a result, xivapi was only returning 100 records (and that's why we only have 100 zones in the file currently). This default limit is referenced here. There are actually 861 records from the World endpoint, so we've never imported the bulk of them. Most are trash/dev/test zones, but we have been omitting a few valid public worlds.

The new xivapi library picks up all records, but the SNR here is not ideal. Two possible options:

  1. Import everything, que sera sera. But we should add the isPublic flag so we have a way to differentiate. (This is the PR currently)
  2. Use isPublic=true to filter the output. This is much more manageable and adds the missing zones we want, but it does drop trash worlds that are in the current file. (With this approach, we don't need to set isPublic in the output, as it's implicit). I'm attaching the output for this option in case anyone wants to run a diff.
    world_id_option_2.txt

I lean toward option 2, since I think that covers our use cases. If that's the consensus, I'll do a quick fix before we land the PR.
(cc: @valarnin - I think you last worked on gen_world_id for raidemulator. Any opinion?)

@valarnin
Copy link
Collaborator

valarnin commented Jan 1, 2024

The new xivapi library picks up all records, but the SNR here is not ideal. Two possible options:

1. Import everything, que sera sera.  But we should add the isPublic flag so we have a way to differentiate.  (This is the PR currently)

2. Use `isPublic=true` to filter the output.  This is much more manageable and adds the missing zones we want, but it does drop trash worlds that are in the current file. (With this approach, we don't need to set isPublic in the output, as it's implicit).  I'm attaching the output for this option in case anyone wants to run a diff.

I don't have time to review the actual script changes today, but I can offer feedback on this.

Are/were the cloud data centers marked as isPublic=true? If so, I'm fine with only including that data set. My only concern is that a transient/test world setup like those were, would not be included.

@wexxlee
Copy link
Collaborator Author

wexxlee commented Jan 1, 2024

Are/were the cloud data centers marked as isPublic=true? If so, I'm fine with only including that data set. My only concern is that a transient/test world setup like those were, would not be included.

They are set to isPublic=true and would be included. Here's the relevant entries:

  '3000': {
    'dataCenter': {
      'id': 12,
      'name': 'NA Cloud DC (Beta)',
    },
    'id': 3000,
    'internalName': 'Cloudtest01',
    'isPublic': true,
    'name': 'Cloudtest01',
    'region': 1,
    'userType': 9,
  },
  '3001': {
    'dataCenter': {
      'id': 12,
      'name': 'NA Cloud DC (Beta)',
    },
    'id': 3001,
    'internalName': 'Cloudtest02',
    'isPublic': true,
    'name': 'Cloudtest02',
    'region': 1,
    'userType': 9,
  },

Given your comments, I'll go ahead and push the commit for option 2.

@github-actions github-actions bot added raidboss /ui/raidboss module oopsy /ui/oopsyraidsy module labels Jan 1, 2024
@wexxlee wexxlee enabled auto-merge (rebase) January 1, 2024 18:15
@github-actions github-actions bot removed raidboss /ui/raidboss module oopsy /ui/oopsyraidsy module labels Jan 2, 2024
util/gen_pet_names.ts Outdated Show resolved Hide resolved
util/xivapi.ts Outdated Show resolved Hide resolved
@wexxlee
Copy link
Collaborator Author

wexxlee commented Jan 4, 2024

@valarnin Changes as discussed. All scripts re-run, and as expected, no data changes. Thanks for the feedback.

The changeover from {} to [] in the xivapi library did break the "error-handling" (such as it was) in gen_effect_id. I have another PR queued up that I'm ready to submit after this lands that adds more robust logging/error handling, So I've quick-fixed it for now by removing the data lookup from printError rather than fixing it just to squash it again.

Copy link
Collaborator

@valarnin valarnin left a comment

Choose a reason for hiding this comment

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

This looks good now. We should make sure to merge the follow up PR before pushing an actual release though, just so it's included in the generated tag.

auto-merge was automatically disabled January 4, 2024 19:40

Rebase failed

@wexxlee wexxlee merged commit 8c976bc into OverlayPlugin:main Jan 4, 2024
5 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 4, 2024
)

Util cleanup project, part 2 of 3 (ref: #26)

This updates the remaining `util/gen_(data)` scripts to use the new
xivapi library. I've deleted `coinach.ts`, since we are no longer
reading SaintCoinach data directly and the file-writer function is now
in the xivapi library.

All 3 scripts were run and updated output files are included. The
changes in `effect_id.ts` and `pet_names.ts` are routine; the underlying
game data has updated since the last time these scripts were run. (These
same changes occur if you run the unmodified scripts.)

The `world_ids.ts` file has substantial changes. The original xivapi
call in that script did not include a `limit=` param in the url; as a
result, xivapi was only returning 100 records (and that's why we only
have 100 zones in the file currently). This default limit is referenced
[here](https://xivapi.com/docs/Game-Data). There are actually 861
records from the `World` [endpoint](https://xivapi.com/World), so we've
never imported the bulk of them. Most are trash/dev/test zones, but we
have been omitting a few valid public worlds.

The new xivapi library picks up all records, but the SNR here is not
ideal. Two possible options:
1. Import everything, que sera sera. But we should add the isPublic flag
so we have a way to differentiate. (This is the PR currently)
2. Use `isPublic=true` to filter the output. This is much more
manageable and adds the missing zones we want, but it does drop trash
worlds that are in the current file. (With this approach, we don't need
to set isPublic in the output, as it's implicit). I'm attaching the
output for this option in case anyone wants to run a diff.

[world_id_option_2.txt](https://github.com/OverlayPlugin/cactbot/files/13804000/world_id_option_2.txt)

I lean toward option 2, since I think that covers our use cases. If
that's the consensus, I'll do a quick fix before we land the PR.
(cc: @valarnin - I think you last worked on `gen_world_id` for
raidemulator. Any opinion?) 8c976bc
@wexxlee wexxlee deleted the util-xivapi-replace branch January 5, 2024 04:47
wexxlee added a commit that referenced this pull request Jan 5, 2024
PR 3 of 3 (ref: #26 & #28)

A handful of misc. improvements:
- More robust console logging functionality to the util data scripts and
xivapi library
- Adds the 3 de-pythoned scripts to the `npm run util` CLI utility
(along with a run-all option), and allows the user to specify a logging
level to control console output
 - Better error capture throughout
- Some minor code cleanup (mostly standardization around use of
CSV/locale tables)
- Corresponding docs updates to `util/README.md` and
`docs/PatchUpdateChecklist.md`

All scripts re-run, no data changes (as expected).

This completes the work I had planned on this project, but in the course
of pulling all this together, I wrote a super-lightweight CLI utility
(integrated into `npm run util` ) to run custom queries against xivapi,
along with some very basic filtering capability. It came in very handy
for investigating data issues, and I could see it being useful if (when
👀 ) future patches create new data collisions or other issues. It needs
some cleanup before it's ready for code review, but I'll likely add that
as a future PR, time permitting.
github-actions bot pushed a commit that referenced this pull request Jan 5, 2024
PR 3 of 3 (ref: #26 & #28)

A handful of misc. improvements:
- More robust console logging functionality to the util data scripts and
xivapi library
- Adds the 3 de-pythoned scripts to the `npm run util` CLI utility
(along with a run-all option), and allows the user to specify a logging
level to control console output
 - Better error capture throughout
- Some minor code cleanup (mostly standardization around use of
CSV/locale tables)
- Corresponding docs updates to `util/README.md` and
`docs/PatchUpdateChecklist.md`

All scripts re-run, no data changes (as expected).

This completes the work I had planned on this project, but in the course
of pulling all this together, I wrote a super-lightweight CLI utility
(integrated into `npm run util` ) to run custom queries against xivapi,
along with some very basic filtering capability. It came in very handy
for investigating data issues, and I could see it being useful if (when
👀 ) future patches create new data collisions or other issues. It needs
some cleanup before it's ready for code review, but I'll likely add that
as a future PR, time permitting. 2c7b245
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.

2 participants