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

Make the name field in projects optional #870

Merged
merged 9 commits into from
Feb 21, 2024

Conversation

Dekkonot
Copy link
Member

@Dekkonot Dekkonot commented Feb 20, 2024

Closes #858.

If a project is named default.project.json, it acts as an init file and gains the name of the folder it's inside of. If it is named something other than default.project.json, it gains the name of the file with .project.json trimmed off. So e.g. foo.project.json becomes foo.

@Dekkonot
Copy link
Member Author

Backport for 7.4.x is here: #871

Copy link
Member

@kennethloeffler kennethloeffler left a comment

Choose a reason for hiding this comment

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

Looking good, just had a small question about ServeSession::project_name and then I will approbe

Comment on lines +222 to +225
self.root_project
.name
.as_ref()
.expect("all top-level projects must have their name set")
Copy link
Member

Choose a reason for hiding this comment

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

Is this panic reachable?

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't be. I gave it a separate warning just in case though.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I'm approving this then and we'll have @osyrisrblx check out #871 before merging since that's the one that's shipping right now

kennethloeffler pushed a commit that referenced this pull request Feb 21, 2024
Unlike most of the other backports, this code couldn't be directly
translated so it had to be re-implemented. Luckily, it is very simple.
This implementation is a bit messy and heavy handed with potential
panics, but I think it's probably fine since file names that aren't
UTF-8 aren't really supported anyway. The original implementation is a
lot cleaner though.

The test snapshots are (almost) all identical between the 7.5
implementation and this one. The sole exception is with the path in the
`snapshot_middleware::project` test, since I didn't feel like adding a
`name` parameter to `snapshot_project` in this implementation.
@kennethloeffler kennethloeffler merged commit 48bb760 into rojo-rbx:master Feb 21, 2024
6 checks passed
@Dekkonot Dekkonot deleted the optional-project-names branch February 21, 2024 01:26
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.

Optional top-level default.project.json "name" field
2 participants