Skip to content

Conversation

@edayot
Copy link
Contributor

@edayot edayot commented Apr 4, 2024

The function with an empty name .mcfunction is currently not supported by beet atm.

This PR fixes this issue by modifying the code where the extensions of a file are calculated (in beet/library/utils.py).

Q/A

  • Where does the problem come from ? : In the python pathlib, the first suffix is skip if the filename start with a dot

  • Why is it a problem ? : Because minecraft loads these functions as usual, and other datapackers rely on this behavior.

@edayot edayot marked this pull request as ready for review April 4, 2024 15:00
@edayot edayot marked this pull request as draft April 4, 2024 15:02
@edayot edayot marked this pull request as ready for review April 4, 2024 15:34
@vberlier
Copy link
Member

vberlier commented Apr 11, 2024

Arf, I apologize for leaving your issue unanswered for so long! I remember that back when I first saw the issue, I thought that allowing empty names could cause problems in other places because beet plugins often just assume that the name can't be empty. I wanted to look into it more in depth but eventually I just forgot and never got back to you.

I still don't have a clear answer though. I've always felt like Mojang doesn't even know that empty names are allowed. If it was never intended, they could very well make it an error the moment they realize and a bunch of packs would break. It just seems like a bug that's bound to be fixed sooner or later so I generally avoid relying on it.

But it's true that as a build tool, beet should strictly conform to the game's behavior. I'll have to look through a couple plugins to see if additional changes are required to take into account empty names.

@edayot
Copy link
Contributor Author

edayot commented Apr 11, 2024

Yeah it always seemed very hacky to me to use empty filenames, just writing function namespace: feels weird, but people are using it.
Do i have to push the files in test/snapshots/** to make the test pass ?

@vberlier
Copy link
Member

Yeah you have to commit the snapshots

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