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

Add a new syncRules field project files to allow users to specify middleware to use for files #813

Merged
merged 43 commits into from
Jan 20, 2024

Conversation

Dekkonot
Copy link
Member

This PR adds the ability to specify a snapshot middleware to use for files that match a given glob, essentially allowing user-customization of Rojo's snapshotting system. For better UX, the name used for this feature is Sync Rules since it aligns with the concepts we use elsewhere (Transformer Rules would also work but I worry that it would imply we were modifying the contents of files), rather than anything mentioning middleware or snapshotting.

An explanation of the syntax is provided in the changelog.

The bulk of this implementation is the process of generalizing the snapshot middleware to not rely upon file names so heavily. This behavior is still present in init files, since the semantics of changing sync rules for those is unclear (should it just be every file named init if it matches a middleware rule?). A suffix field is included to support multiple file extensions like Rojo uses, which means that everything we currently do (minus .meta.json files) is representable as a syncing rule.

My motivation for this feature is two-fold:

  1. A more rough implementation of this is used at Uplift Games to sync our wally.lock and git ref information into Adopt Me for the sake of build integrity and identification. It's a blocker for us updating Rojo, and I would like to get it added here so that any subsequent changes I make at Uplift can be pushed directly to Rojo rather than to our fork.

  2. The existence of syncing rules makes it more practical to change file extensions or support per-script RunContext adjustments since there's a way to maintain compatibility now. It may be worth introducing new variants of the Middleware enum for RunContext for scripts to support users specifying their own extensions to support it.

I'm not 100% on the names of the middleware, but I'm open to feedback on them.

@Dekkonot Dekkonot added size: medium impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. labels Oct 31, 2023
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 pretty ok - I need to go feed myself right now, but I'll be back shortly for a closer look at snapshot_middleware/mod.rs. A few comments:

src/snapshot/metadata.rs Outdated Show resolved Hide resolved
src/snapshot_middleware/project.rs Outdated Show resolved Hide resolved
src/snapshot_middleware/mod.rs Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/snapshot/metadata.rs Outdated Show resolved Hide resolved
src/snapshot/metadata.rs Outdated Show resolved Hide resolved
src/snapshot_middleware/lua.rs Show resolved Hide resolved
}

Ok(None)
}
Copy link
Member

Choose a reason for hiding this comment

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

(no action needed, just a comment)

Similar to Ken's comment later in this file, I'm thinking about if there's a better way to do this.

It would be cool if we could support user-defined init types, but that seems like a bit of a mess. It would let us move all of this logic to default sync rules, though! I wonder if there's a way we can ergonomically define priorities for these things with a future version of sync rules, such that we just have internal default sync rules that maintain this priority?

src/snapshot_middleware/mod.rs Outdated Show resolved Hide resolved
src/snapshot_middleware/mod.rs Show resolved Hide resolved
Copy link
Member

@Corecii Corecii left a comment

Choose a reason for hiding this comment

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

Two potential minor improvements that aren't blockers -- fix them or leave them up to you!

src/snapshot_middleware/mod.rs Outdated Show resolved Hide resolved
src/snapshot_middleware/mod.rs Show resolved Hide resolved
Copy link
Contributor

@chriscerie chriscerie left a comment

Choose a reason for hiding this comment

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

Overall looks good

src/snapshot/metadata.rs Outdated Show resolved Hide resolved
Copy link
Member

@Corecii Corecii left a comment

Choose a reason for hiding this comment

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

still looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: medium Moderate issue for Rojo users or a large issue with a reasonable workaround. size: medium type: enhancement Feature or improvement that should potentially happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants