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

Protect against syncing and building the wrong project types #772

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

Conversation

Dekkonot
Copy link
Member

@Dekkonot Dekkonot commented Aug 14, 2023

Closes #736.

Prevents projects with a data model as a root from being built as a model file and vice versa. Also prevents syncing model files altogether.

This should mitigate a few of the more common problems with building Rojo projects. I'm not sure whether I should revert #691 or not. The check it adds is made moot with this PR, but it could be a good failsafe. Open to suggestion.

This doesn't impact the VSCode extension at all because it determines the file type to build as using a similar heuristic as this PR and listens to errors during serving to display to the user.

@Dekkonot Dekkonot added type: enhancement Feature or improvement that should potentially happen scope: cli Relevant to the Rojo CLI labels Aug 14, 2023
@Dekkonot
Copy link
Member Author

You know, that's on me I didn't think to test whether our own tests would break if we changed this, I will fix it soon:tm:. It is currently 80 degrees and my laptop is hot though, so it'll have to wait.

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.

The fact that tests were relying on this behavior is pretty funny! 😂

While technically a breaking change, I doubt anyone ever actually intended to do this, or got a working result out of it... so it's probably fine?

src/cli/serve.rs Outdated Show resolved Hide resolved
src/cli/serve.rs Outdated Show resolved Hide resolved
@boatbomber
Copy link
Member

Regarding #691, I think leaving that protection in the plugin is still valuable both as a failsafe and as protection against serves from otherwise compatible <=7.3.0 servers.

@Dekkonot
Copy link
Member Author

We can revisit tests later, since I'm not sure on the names of some of these. I want to keep the diff as relevant as possible though, since this turned into a bigger patch than anticipated.

@Dekkonot Dekkonot requested review from kennethloeffler and boatbomber and removed request for kennethloeffler August 20, 2023 19:28
@Dekkonot
Copy link
Member Author

..Apologies for whatever the hell notification that just sent you, Ken. Github's UI isn't intuitive for this.

src/cli/serve.rs Show resolved Hide resolved
tests/tests/build.rs Show resolved Hide resolved
@Dekkonot Dekkonot requested review from kennethloeffler and removed request for kennethloeffler September 21, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: cli Relevant to the Rojo CLI type: enhancement Feature or improvement that should potentially happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent building model as a place and vice versa
3 participants