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.

Sign up for GitHub

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

Jump to bottom

Factorio: update API use #3760

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Factorio: update API use #3760

merged 3 commits into from
Sep 17, 2024

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

updates various Factorio API use to current AP standards.

How was this tested?

unittests and a single local gen

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 10, 2024
@Exempt-Medic Exempt-Medic added is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. is: refactor/cleanup Improvements to code/output readability or organizization. labels Aug 10, 2024
@NewSoupVi NewSoupVi mentioned this pull request Aug 10, 2024
Open
65 tasks
@NewSoupVi
Copy link
Member

Thank you 🎉

Exempt-Medic
Exempt-Medic approved these changes Aug 11, 2024
View reviewed changes
Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

The changes LGTM. Generations are completely different, presumably because of the random changes, but the playthrough seems similar (I don't know the game really at all though, just going off of item names). A few minor comments.

Exempt-Medic
Exempt-Medic approved these changes Aug 11, 2024
View reviewed changes
Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Random generations didn't hit any old options stuff and I didn't find any with a search. Generations before and after are consistent if the random changes are reverted. Comments were fully addressed

qwint
qwint approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

pure code review; did not run gens/tests/etc., did not check for completeness

updates all looks fine, got a little worried about changing the 'world' parameter in technology.get_custom() to the World instance instead of MultiWorld, but digging through that codepath it seems to be fine/correct

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Aug 23, 2024
@Berserker66
Merge branch 'refs/heads/main' into factorio_revamp
6f0f8fa 
# Conflicts:
#	worlds/factorio/__init__.py
@Berserker66 Berserker66 merged commit 6fac83b into main Sep 17, 2024
30 checks passed
@Berserker66 Berserker66 deleted the factorio_revamp branch September 17, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silasary silasary silasary approved these changes

@qwint qwint qwint approved these changes

@NewSoupVi NewSoupVi NewSoupVi approved these changes

@Exempt-Medic Exempt-Medic Exempt-Medic approved these changes

Assignees
No one assigned
Labels
is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Berserker66 @NewSoupVi @silasary @qwint @Exempt-Medic