Skip to content

Conversation

CarabusX
Copy link
Contributor

Helps in cases of endless unloading at the depot.

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

Code looks good! "rejoin_pack_animals" is kind of a long name, but it is specific and understandable. Calling it just "fix" is an option, and is "forward compatible" in case there are more things we can check for and fix in the future. @lethosor, what do you think?

Could you add a changelog entry in changelog.txt that describes this change? It should go in the "Misc Improvements" section of the "Future" release.

@myk002
Copy link
Member

myk002 commented Jun 10, 2022

Also, please consider adding your name to the https://github.com/DFHack/dfhack/blob/develop/docs/Authors.rst file in the DFHack/dfhack repo.

@CarabusX
Copy link
Contributor Author

CarabusX commented Jun 11, 2022

Code looks good! "rejoin_pack_animals" is kind of a long name, but it is specific and understandable. Calling it just "fix" is an option, and is "forward compatible" in case there are more things we can check for and fix in the future. @lethosor, what do you think?

Yeah name is kinda long, thats why I called it "rejoin" and not "reconnect".
In case there are more things to fix in the future, it may be good to have separate commands for that, so maybe better if current command is specific?

Could you add a changelog entry in changelog.txt that describes this change? It should go in the "Misc Improvements" section of the "Future" release.

Sure! I will add it there.

Edit: Done.

@CarabusX
Copy link
Contributor Author

CarabusX commented Jun 11, 2022

Also, please consider adding your name to the https://github.com/DFHack/dfhack/blob/develop/docs/Authors.rst file in the DFHack/dfhack repo.

Sure! I will add myself there later.

Edit: Done. Made a PR,

@CarabusX CarabusX force-pushed the master branch 2 times, most recently from 1738707 to 157f1c3 Compare June 11, 2022 12:44
Copy link
Member

@lethosor lethosor left a comment

Choose a reason for hiding this comment

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

I agree with @myk002 that the name is kind of long. I think it's also more specific than it needs to be. From what I understand, it took you some time to figure out that your caravan was stuck because the pack animals were disconnected. I don't think we can expect users to figure that out. From their perspective, the caravan is just stuck, and they don't know why, so I think unstick or something similar would be better (and shorter).

Now, if we want to maintain the ability to apply specific, targeted fixes in the future if unstick does more than one thing, I would be in favor of making unstick() call out to rejoin_pack_animals() and whatever else is necessary to get a caravan unstuck.

CarabusX and others added 3 commits June 13, 2022 15:07
Helps in cases of endless unloading at the depot.
Co-authored-by: Myk <myk002@yahoo.com>
@CarabusX
Copy link
Contributor Author

CarabusX commented Jun 13, 2022

I agree with @myk002 that the name is kind of long. I think it's also more specific than it needs to be. From what I understand, it took you some time to figure out that your caravan was stuck because the pack animals were disconnected. I don't think we can expect users to figure that out. From their perspective, the caravan is just stuck, and they don't know why, so I think unstick or something similar would be better (and shorter).

Now, if we want to maintain the ability to apply specific, targeted fixes in the future if unstick does more than one thing, I would be in favor of making unstick() call out to rejoin_pack_animals() and whatever else is necessary to get a caravan unstuck.

I will call it unload then, because it is only about caravan not unloading.
unstick seems too general - is the caravan stuck moving, or entering map, or leaving map, or unloading goods?

I will make unload() call out to rejoin_pack_animals() internally.

Edit: Done.

@CarabusX CarabusX changed the title Implemented new 'rejoin_pack_animals' command in caravan script. Implemented new 'unload' command in caravan script. Jun 13, 2022
@CarabusX CarabusX requested a review from lethosor June 13, 2022 13:47
CarabusX added a commit to CarabusX/dfhack that referenced this pull request Jun 13, 2022
lethosor pushed a commit to DFHack/dfhack that referenced this pull request Jun 14, 2022
@myk002 myk002 merged commit ce5c7cb into DFHack:master Jun 17, 2022
@myk002
Copy link
Member

myk002 commented Jun 17, 2022

btw, your master branch will be out of sync after this is merged. you'll want to

git fetch upstream
git checkout master
git reset --hard upstream/master
git push --force

to get back in sync. Creating branches for PRs will avoid this issue in the future.

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.

3 participants