-
Notifications
You must be signed in to change notification settings - Fork 182
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
Conversation
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.
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:
} | ||
|
||
Ok(None) | ||
} |
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.
(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?
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.
Two potential minor improvements that aren't blockers -- fix them or leave them up to you!
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.
Overall looks good
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.
still looks good to me!
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 namedinit
if it matches a middleware rule?). Asuffix
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:
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.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.