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

fix: skip out of date file changes #1279

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Conversation

wesbillman
Copy link
Collaborator

Fixes #1210

This required a bit of a refactor to use pointers for Module and ExternalLibrary here. Hopefully, that's ok.

Example of rapid changes getting skipped. Note that I logged skips in Warnf just to highlight for this screenshot. Real skip messages are logged at Debugf

Screenshot 2024-04-16 at 2 03 09 PM

@wesbillman wesbillman requested a review from a team as a code owner April 16, 2024 21:13
@wesbillman wesbillman requested review from worstell and removed request for a team April 16, 2024 21:13
@alecthomas alecthomas mentioned this pull request Apr 16, 2024
switch project := project.(type) {
case Module:
func Build(ctx context.Context, sch *schema.Schema, proj Project) error {
switch project := proj.(type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can and should just do switch project := project.(type) {

case Module:
func Build(ctx context.Context, sch *schema.Schema, proj Project) error {
switch project := proj.(type) {
case *Module:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the pointer change?

@@ -285,12 +285,19 @@ func (e *Engine) watchForModuleChanges(ctx context.Context, period time.Duration
delete(e.projects, config.Key)
case WatchEventProjectChanged:
config := event.Project.Config()

lastBuildTime := e.projects[config.Key].GetLastBuildStartTime()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think rather than making all these changes because of making Project mutable, just change the e.projects map to use an intermediate struct that contains the Project + time.Time:

type projectMeta struct {
  project Project
  lastBuildStartTime time.Time
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Love it! Thanks dude!

@wesbillman wesbillman force-pushed the skip-out-of-date-file-changes branch from 09245b5 to eaac130 Compare April 16, 2024 22:09
Comment on lines +35 to +38
type projectMeta struct {
project Project
lastBuildStartTime time.Time
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alecthomas I moved away from embedding because it seems to cause more confusion than help. For example some of the methods that took in a project Project would be ok with a projectMeta but then the type assertions would require a projectMeta case and I was getting lots of bugs where I should be using project.project but I was just using project directly. lemme know if this seems reasonable :)

@wesbillman wesbillman merged commit 1f04045 into main Apr 16, 2024
11 checks passed
@wesbillman wesbillman deleted the skip-out-of-date-file-changes branch April 16, 2024 22:16
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.

File updates in quick succession leads to unnecessary builds
2 participants