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

Sort operations to guarantee a stable order #156

Merged
merged 4 commits into from
Nov 26, 2021
Merged

Sort operations to guarantee a stable order #156

merged 4 commits into from
Nov 26, 2021

Conversation

edigaryev
Copy link
Contributor

Problem: currently when using a wildcard that covers multiple files in operations: YAML directive the generator emits functions in a non-stable ordering due to the use of map for storing file names:

func expandFilenames(globs []string) ([]string, error) {
uniqFilenames := make(map[string]bool, len(globs))
for _, glob := range globs {
matches, err := filepath.Glob(glob)
if err != nil {
return nil, errorf(nil, "can't expand file-glob %v: %v", glob, err)
}
for _, match := range matches {
uniqFilenames[match] = true
}
}
filenames := make([]string, 0, len(uniqFilenames))
for filename := range uniqFilenames {
filenames = append(filenames, filename)
}
return filenames, nil
}

Solution: sort the operations before emitting them.

@csilvers
Copy link
Member

Good catch! Change looks good to me. The only issue before merging is to make sure you've signed the CLA. Sadly I don't know the details of that. I'll try to figure out out after the holiday (probably Monday)

@csilvers
Copy link
Member

Oh, one more thing: it looks like there's a failing test. Can you fix that up?

@benjaminjkraft
Copy link
Collaborator

Good idea -- matches what we do for the types they reference. Thanks!

The CLA is at https://www.khanacademy.org/r/cla. For the tests I think you'll just need to go generate ./... to rebuild the example and integration tests.

@edigaryev
Copy link
Contributor Author

Oh, one more thing: it looks like there's a failing test. Can you fix that up?

Fixed a couple of tests failing locally, see b9c47e4 and 95c5403.

Not sure about the tests that run on CI, though.

@edigaryev
Copy link
Contributor Author

For the tests I think you'll just need to go generate ./... to rebuild the example and integration tests.

See 6930225.

@edigaryev
Copy link
Contributor Author

The CLA is at https://www.khanacademy.org/r/cla.

CLA sent.

@benjaminjkraft benjaminjkraft merged commit f80df6d into Khan:main Nov 26, 2021
@edigaryev edigaryev deleted the sort-operations branch November 26, 2021 20:32
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.

3 participants