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

Allow absolute paths in schema files. #192

Merged
merged 3 commits into from
May 12, 2022
Merged

Allow absolute paths in schema files. #192

merged 3 commits into from
May 12, 2022

Conversation

csilvers
Copy link
Member

Thsi is useful for some out-of-tree testing I want to do. It's
unfortunate (imo) that filepath.join doesn't have this behavior by
default.

Test plan:
go test ./...

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

Thsi is useful for some out-of-tree testing I want to do.  It's
unfortunate (imo) that filepath.join doesn't have this behavior by
default.

Test plan:
go test ./...
// pathJoin is like filepath.Join but 1) it only takes two argsuments,
// and b) if the second argument is an absolute path the first argument
// is ignored (similar to how python's os.path.join() works).
func pathJoin(a, b string) string {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest a more accurate name such as func abs(basedir, path string) string {
This is then similar to the stdlib's filepath.Abs but allows you to specify the base directory instead of using the current working directory.

func Abs
func Abs(path string) (string, error)
Abs returns an absolute representation of path. If the path is not absolute it will be joined with the current working directory to turn it into an absolute path. The absolute path name for a given file is not guaranteed to be unique. Abs calls Clean on the result.

Copy link
Member Author

Choose a reason for hiding this comment

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

abs isn't very accurate because there's no requirement that a be an absolute path. I really see this as a "proper" path-join (and the default go implementation as not-proper). But ymmv.

Copy link
Member

Choose a reason for hiding this comment

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

This is an unexported helper, so you do you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree the name isn't ideal, but I don't have a better idea, so I'm fine with this. (Closest thing I can think of in the stdlib is net/url.ResolveReference which I don't think is the greatest name either.)

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

}
for i := range c.Operations {
c.Operations[i] = filepath.Join(baseDir, c.Operations[i])
c.Operations[i] = pathJoin(baseDir, c.Operations[i])
}
c.Generated = filepath.Join(baseDir, c.Generated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

May as well do Generated too!

// pathJoin is like filepath.Join but 1) it only takes two argsuments,
// and b) if the second argument is an absolute path the first argument
// is ignored (similar to how python's os.path.join() works).
func pathJoin(a, b string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree the name isn't ideal, but I don't have a better idea, so I'm fine with this. (Closest thing I can think of in the stdlib is net/url.ResolveReference which I don't think is the greatest name either.)

@csilvers csilvers merged commit 482a59b into main May 12, 2022
@csilvers csilvers deleted the abspath-in-schema branch May 12, 2022 21:04
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