Skip to content

Conversation

@fitzgen
Copy link
Contributor

@fitzgen fitzgen commented Aug 29, 2015

A series of cleanups and follow ups to #205. See each commit for details.

r? @jlongster

@sokra
Copy link
Contributor

sokra commented Sep 1, 2015

I personally don't like that generated files (dist/) are commited into the repo.

@fitzgen
Copy link
Contributor Author

fitzgen commented Sep 1, 2015

I personally don't like that generated files (dist/) are commited into the repo.

Can you expand on why?

I agree that it isn't the most "clean" because they aren't part of the source, but instead artifacts created from the source. However, this enables people to just download a built version of the library without building themselves (#179) and uses .gitattributes to mark the files as binary so that it only displays whether they changed or not in diffs rather than dumping the duplicate/irrelevant changes out with the rest of the diff.

@sokra
Copy link
Contributor

sokra commented Sep 2, 2015

You'll get inconsitent PRs (some with generated files change, some without). PRs conflict more often because generated files differ (with binary files even it's even more likely).

If you want to offer a download to the generated file, you could keep the generated files in a gh-pages branch and use Continuous Delivery (i. e. Codeship) to build them automatically.

@jlongster
Copy link

fwiw, I keep the dist files in the nunjucks repo while it is nice, I do run into problems. Sometimes I forget to regenerate them, or I accept PRs but don't regenerate them and commit back, etc.

If there's a way to automatically build and push to gh-pages whenever a new commit lands to master, I'd say do that and just link directly to those JS files from the README. I'm not sure if you can do that without a complicated CI setup though, which may not be worth it for this project. If it's a simple webhook or something like that though, I think it's a good idea.

Choose a reason for hiding this comment

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

Is the only reason for this so the github interface doesn't show it? I don't really think it's worth it; it would make conflicts harder to figure out, which is more likely with the dist files committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes git diff not spew changes for that file as well. I think it is worth it since the best way to handle conflicts in the generated files is to kill them and regenerate them.

@jlongster
Copy link

Oh looks like you're already using TravisCI. I definitely think we should generate builds from there and just link to them in the README.

README.md Outdated

Choose a reason for hiding this comment

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

missing end quote

@jlongster
Copy link

r+, but I agree about using TravisCI to use builds. Can we look into that?

@fitzgen
Copy link
Contributor Author

fitzgen commented Sep 10, 2015

I think we could use http://docs.travis-ci.com/user/deployment/releases/ to have travis attach the build files on each release, but I'm not sure what the implications of attaching a token to a public repo are in this specific case (and so I am very hesitant).

Move the module entry point from `./lib/source-map.js` -> `./source-map.js`.

Move `./lib/source-map/*` -> `./lib/*`.

Move `./test/source-map/*` -> `./test/*`
This adds the files generated by the build under `dist/*` to the repoistory
proper. This let's us distribute the built versions of the library via github's
raw links instead of telling people to build from source themselves.
This commit moves information about building and testing from the README.md to
the new CONTRIBUTING.md.
fitzgen added a commit that referenced this pull request Sep 10, 2015
@fitzgen fitzgen merged commit dd77a2f into mozilla:master Sep 10, 2015
@jlongster
Copy link

I know other projects do this somehow, even if it's just making the links for dist files straight from travis-ci.com. Might be worth asking another project at least (I can't actually think of one off-hand...). But it's not a big deal, though in my experience it does get annoying over time.

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