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

Refactor pocket #79305

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Refactor pocket #79305

wants to merge 3 commits into from

Conversation

Brambor
Copy link
Contributor

@Brambor Brambor commented Jan 23, 2025

Summary

None

Purpose of change

While trying to understand the code, I found some things to improve.

Describe the solution

  1. (1st commit) rename parameter is_pick_up_inv to ignore_non_container_pocket and document it.
  2. (2nd commit) Make it a single, translatable sentence. If there is a language that uses periods differently, it can do so in this menu as well. It was not done before for historical reasons.
  3. (3rd commit) Unimplemented parameter with no docs of what it does, used nowhere. This PR removes it.

Describe alternatives you've considered

  1. Just document is_pick_up_inv instead of renaming it. The doc comment would confuse the programmer as to why the parameter has such a weird name.

Testing

  1. Compiles.
  2. Sentence is still right (has period at the end, there is no weird spacing):
    • image

Additional context

More refactoring is possible, but I plan to change the "Can be stored in" menu. Then the code will change too.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Jan 23, 2025
@Brambor Brambor marked this pull request as draft January 23, 2025 20:08
@Brambor Brambor marked this pull request as ready for review January 23, 2025 20:09
@Brambor
Copy link
Contributor Author

Brambor commented Jan 24, 2025

I believe the test failure is unrelated https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/12937209329/job/36084633496?pr=79305.

Restarting tests.

@Brambor Brambor marked this pull request as draft January 24, 2025 01:34
@Brambor Brambor marked this pull request as ready for review January 24, 2025 01:34
@Brambor Brambor marked this pull request as draft January 24, 2025 08:43
@Brambor Brambor marked this pull request as ready for review January 24, 2025 08:44
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Items: Containers Things that hold other things json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant