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

Support static files #45

Merged
merged 6 commits into from
Oct 13, 2017
Merged

Support static files #45

merged 6 commits into from
Oct 13, 2017

Conversation

gernest
Copy link
Contributor

@gernest gernest commented Oct 3, 2017

This adds support for static files. You can add files into a directory that is defined by StaticDir in gnorm.toml and they will be copied to the directory defined by OutputDir in the gnorm.toml.

The structure of the directories is preserved.

Closes #14

This adds support for static files. You can add files into a directory that  is defined by `StaticDir` in  gnorm.toml and they will be copied to the directory defined by `OutputDir` in the gnorm.toml.

The structure of the directories is preserved.

Closes gnormal#14
@natefinch
Copy link
Member

ooh, I missed this. See, this is why I need the channel :)

Copy link
Member

@natefinch natefinch left a comment

Choose a reason for hiding this comment

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

Looking good, if you can fix up a few things. Oh, and don't forget to update the website.

cli/parse.go Outdated
@@ -50,6 +50,13 @@ func parse(env environ.Values, r io.Reader) (*run.Config, error) {
if len(c.ExcludeTables) > 0 && len(c.IncludeTables) > 0 {
return nil, errors.New("both include tables and exclude tables")
}
if c.OutputDir == "" {
wd, err := os.Getwd()
Copy link
Member

Choose a reason for hiding this comment

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

in basically all cases, "." should just work here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will that work even on windows?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, it works. But definitely good to be thinking about windows. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perfect! Yah, windows folks need some love.

@@ -107,4 +107,17 @@ type Config struct {
// PluginDirs a set of absolute/relative paths that will be used for
// plugin lookup.
PluginDirs []string

// OutputDir is the directory relative to the project root (where the
// gnorm.toml file is located) in which all the generated files are written
Copy link
Member

Choose a reason for hiding this comment

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

mention this defaults to "."

}
}
if !dstat.IsDir() {
return fmt.Errorf("%s is not a directory", dest)
Copy link
Member

Choose a reason for hiding this comment

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

can you make this "Outputdir specifies a directory path that already exists as file %q" . or something like that? it'll make the error more clear to the user.

run/generate.go Outdated
}
dirs[o] = true
}
d, err := ioutil.ReadFile(path)
Copy link
Member

Choose a reason for hiding this comment

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

Don't read the whole file into memory, in theory it could be a 40 gig bluray image. Do this instead:

f, err := os.Open(path)
if err != nil {
    return err
}
defer f.Close()
g, err := os.Cteate(target)
if err != nil {
    return err
}
_, err := io.Copy(g, f) // copy goes right to left, like variable assignment
if err != nil {
    return err
}

}

for _, v := range src {
err := os.MkdirAll(v.path, 0777)
Copy link
Member

Choose a reason for hiding this comment

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

instead of creating the files programmatically, just put them in a directory called testdata - the go tool ignores those files, and then you won't need all this code to create them, and it'll make the code easier to read.


// make sure the structure is preserved
var newPaths []string
filepath.Walk(dest, func(path string, info os.FileInfo, err error) error {
Copy link
Member

Choose a reason for hiding this comment

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

since you have a well-known list of files you're looking for, you can probably use ioutil.ReadDir and the code will be easier to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are checking the order of files in nested directories. ReadDir reads the content of one directories, I think walk here is a better fit rather than calling ReadDil a couple of times which we will also be walking the directory tree.

run/generate.go Outdated
}
o := filepath.Join(dest, rel)
if !dirs[o] {
err = os.MkdirAll(o, stat.Mode())
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 just call mkdirall every time, it won't error out if the directory already exists, and that way you don't have to keep track

@gernest
Copy link
Contributor Author

gernest commented Oct 7, 2017

Done

@gernest
Copy link
Contributor Author

gernest commented Oct 11, 2017

Ping

Copy link
Member

@natefinch natefinch left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm going to do some manual testing to make sure it behaves the way I expect, but I think it's good to go.

@natefinch
Copy link
Member

I tested this, but it's not prepending the outputDir to the templatePath target files. I have a fix for that which I could push to this branch if you give me rights to do it. You just need to check "Allow edits from maintainers" on this PR.

@gernest
Copy link
Contributor Author

gernest commented Oct 13, 2017

Hey, Allow edits from maintainers is checked.

@natefinch
Copy link
Member

weird, I tried to push again, but no luck. still says permission denied. I'll have to poke at it in the morning.

all I did was add cfg.OutputDir to the genfile function,.and filepath.Join it to the generated path.

@gernest
Copy link
Contributor Author

gernest commented Oct 13, 2017

Uh! Maybe you can add that in a separate PR? That was my intention but I forgot to add the comment . This PR was only about copying static files.

@natefinch natefinch merged commit a5c1d6c into gnormal:master Oct 13, 2017
@gernest gernest deleted the static branch October 13, 2017 13:15
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.

2 participants