-
Notifications
You must be signed in to change notification settings - Fork 17
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 and improve exporting #271
Conversation
…efactor exporting
Merge develop to update the branch with new tests
…ehavior of the new local export: Before this update the files were exported into build/RP and build/BP by default but it was inconsistent with other export target behaviors. Now regolith exports into build/<project-name>_rp and build/<project-name>_bp.
… new behavior of the new local export.
…ehavior of the new local export.
…lith into export-refactor
…ior of the new local export.
…behavior of the new local export.
return | ||
} | ||
|
||
func GetExportNames(exportTarget ExportTarget, ctx RunContext) (bpName string, rpName string, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly, that the new bpName
and rpName
properties are evaluated? What's the benefit of that over having a static value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit is the flexibility. You can choose whether is a prefix or a suffix or maybe they want to format the name etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was writing the documentation for this and looked into the code a little bit more and honestly I don't see when this would be useful. The set of exposed variables is weird and one of them is very confusing to me. I'm looking at this part of the code:
regolith/regolith/evaluator.go
Lines 38 to 57 in de85f24
func prepareScope(ctx RunContext) map[string]interface{} { | |
semverString, err := utils.ParseSemverString(Version) | |
if err != nil { | |
semverString = utils.Semver{} | |
} | |
projectData := map[string]interface{}{ | |
"name": ctx.Config.Name, | |
"author": ctx.Config.Author, | |
} | |
return map[string]interface{}{ | |
"os": runtime.GOOS, | |
"arch": runtime.GOARCH, | |
"debug": burrito.PrintStackTrace, | |
"version": semverString, | |
"profile": ctx.Profile, | |
"filterLocation": ctx.AbsoluteLocation, | |
"settings": ctx.Settings, | |
"project": projectData, | |
} | |
} |
- "name" - this is probably the only useful variable on the list. That said, the benefit is tiny. Using it means that if you want to export to a directory named after the project, you don't have to change the value twice when you rename the project.
- "author" - maybe someone would like to export to a directory that includes their name.
- "os" - probably useless
- "arch" - probably useless
- "debug" - I guess someone might want to export to different path when they're debugging. But in what case?
- if you use
development
export it would most likely create multiple packs with the same UUID in your dev packs folder. This could potentially cause problems in Minecraft. - in my experience
local
export target is used only for Regolith development and to showing people examples, but if someone is using some custom workflow wherelocal
is useful then they are probably using some other tool to export the files to the actual location and they probably have that tool set up in such a way that it expects specific export path. exact
doesn't support this featureworld
- same problem as withdevelopment
preview
- same problem as withdevelopment
andworld
- if you use
- "version" - useless. Do you run multiple versions of Regolith?
- "profile" - similar benefit to "name" but probably even smaller.
- "filteLocation" - honestly I don't even know what that is.
- "settings" - same here. I don't know what it does. I think it normally would have filter settings but in context of creating a name of an export directory for a profile there is no single filter that could be used to provide data for this.
- "project" contains the data I discussed in first 2 points.
If you find this feature useful for your specific workflow that's fine. I'm not saying that we have to remove it. TBH it doesn't affect me at all because I use exact
export target, but I don't know what to write in the documentation.
I guess I could mention only the "author", "debug", "name" and "profile" properties. I could also add descriptions for "os" and "arch". The rest of the variables is either useless or I don't understand what they do (which means I can't really document them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated documentation: 23c2696
…' and 'bpName' properties.
…e and bpName properties.
This PR adds
none
export target and addsrpName
andbpName
properties, that let the user change the name of the bp and rp folders.Also I did a minor refactor of the export code.