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

fix: populate context from config #452

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

BPR02
Copy link

@BPR02 BPR02 commented Nov 25, 2024

- populate ctx.data and ctx.assets with the following info from the config:
  - name
  - description
  - pack_format
  - supported_formats
@BPR02
Copy link
Author

BPR02 commented Jan 14, 2025

@misode @RitikShah @edayot I think this PR can be merged. I tested it with my Observer plugin and it seems to be working as expected. I'd like a review if you guys have time.

I added some tests, but there might be a more elegant way to test the output... I'm not super familiar with pytest, but I couldn't find anything online about passing an output to an object for pytest to catch, so I just put the text into a meta field and checked it afterwards.

beet/toolchain/project.py Outdated Show resolved Hide resolved
tests/test_ctx_config.py Show resolved Hide resolved
@misode misode self-requested a review January 21, 2025 20:17
@misode
Copy link
Member

misode commented Jan 21, 2025

@BPR02 I have made a small adjustment: I've moved this copying logic to the ProjectBuilder.bootstrap function, since that is also where the other data_pack and resource_pack loading and outputting is done. To me it also makes sense that this data would only be used after require.

Since the order is currently (1) require -> (2) data|resource_pack load -> (3) data|resource_pack render -> (4) pipeline -> (5) data|resource_pack output. What this PR now does is insert a set between (1) and (2) that copies the data|resource_pack config to the ctx. It could've also been between (2) and (3) but I don't think it makes any difference. It being before (3) is probably useful if you want to use these variables in a jinja template.

Further remarks

  1. We can probably also do this for the part that sets the description_parts (lines 340 to 359, and line 361), which would allow plugins to overwrite the pack description. I vaguely remember encountering such a problem for GM4. Should probably not be in this PR though.
  2. At line 373 and below there is already logic for populating the pack's data from the config. Maybe we can just move part of this logic to the top instead of doing it twice? - Edit: yes, this is a good idea, I will implement this

@misode misode marked this pull request as draft January 22, 2025 00:08
@misode
Copy link
Member

misode commented Jan 22, 2025

I've made some more drastic changes now, and it's not quite done yet, because there is some behavior change, for example when pack metadata is present both in the beet config and in pack.mcmeta.

  • Having the pack.pack_format be set to the beet config pack_format is useful when loading from a folder that doesn't a pack.mcmeta but you still want the right scopes to be used. I am not sure how beet currently handles this (needs to be tested)
  • On the other hand, this means that if pack_format is defined in both the beet config and pack.mcmeta, with this PR pack.mcmeta will take priority (should add a test to cover this).
  • Similarly for both filter and overlays, the entries from pack.mcmeta will be appended after the values from the beet config.

With regards to which config takes priority, I am unsure whether we should keep compatibility with the released beet version, or decide from scratch what the ideal behavior should be.

@BPR02
Copy link
Author

BPR02 commented Feb 5, 2025

I think it makes sense to make the beet config be the "one true source" of configuration values. I'm not sure what the current release behavior is, but for this PR maybe it should be changed to only pull the pack_format from the pack.mcmeta if it's not already defined in the beet config. However I think filter and overlays would make sense to append. Would it be confusing to users if some values are overwritten (e.g. pack_format) while others are extended (e.g. overlays)?

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.

ctx.data and ctx.assets are not populated by the config
3 participants