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

Add .gitignore for new packages #9541

Merged
merged 1 commit into from
Jan 4, 2015
Merged

Add .gitignore for new packages #9541

merged 1 commit into from
Jan 4, 2015

Conversation

garborg
Copy link
Contributor

@garborg garborg commented Jan 1, 2015

Adding the .gitignore: the clutter seems worth it to make code-coverage and track-allocation more painless (/ hopefully common)

Completing Travis script: still somewhat confusing, as the commented script is not the default script, but so long as it's not, might as well make it a complete version of the most commonly used alternative to the default

@garborg
Copy link
Contributor Author

garborg commented Jan 1, 2015

Hmm... looks like a timeout on Windows and failure to download libgit2 on OSX.

@tkelman
Copy link
Contributor

tkelman commented Jan 1, 2015

I'd also put deps/deps.jl in the gitignore, though I'm not sure off the top of my head whether that has to be a separate gitignore file in deps.

@vtjnash
Copy link
Member

vtjnash commented Jan 1, 2015

lgtm. it's optional whether you do them in separate files or not.

i disagree that deps/deps.jl should be ignored by default

@IainNZ
Copy link
Member

IainNZ commented Jan 1, 2015

I'd maybe hold off on adding Coverage/Coveralls.io until JuliaCI/travis-build#1 is resolved

@garborg
Copy link
Contributor Author

garborg commented Jan 1, 2015

Okay, edit to remove coverage=true as well? (doesn't make sense to keep one around without the other)

@IainNZ
Copy link
Member

IainNZ commented Jan 1, 2015

Oh hold on, I just realized all those lines are commented out. Yeah it does seem to be one or the other... maybe needs additional comments to explain what all that Coverage stuff is.

@garborg
Copy link
Contributor Author

garborg commented Jan 1, 2015

Additional comments would be helpful -- not sure where to go with them before JuliaCI/travis-build#1 is resolved, as you said -- guess I should update this then.

Also, thoughts on whether the commented out code ought to match the default script or be a useful alternative (e.g. opposite of the default script in terms of doing code coverage)?

@tkelman
Copy link
Contributor

tkelman commented Jan 1, 2015

The idea of the default script on the travis side is to reflect package-testing best practices. Having the commented-out section there in the generated .travis.yml file is to help people who need to customize their package's testing process or dependencies, to give them something to start from to avoid hitting common pitfalls like the shallow clone issue. So I think having them match makes sense, but I'm not sold on turning on Coveralls by default for all packages until they fix lemurheavy/coveralls-public#25

@@ -149,6 +150,17 @@ function travis(pkg::AbstractString; force::Bool=false)
#script:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clear things up, and since people switching over to the default script are mistakenly thinking that coverage=true is set, how about adding this line here for now?: # (the following snippet differs from the default test script in that coverage/Coveralls are not enabled by default)

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like more of these descriptions should be going in the package development documentation (based on feedback I've seen various places, people seem to be missing that it exists, or it needs to be expanded to cover more use cases) rather than comments in a generated file.

Also I guess I was hoping JuliaCI/travis-build#1 would be resolved a little sooner, in that at least coverage=true could be made the default on Travis before we figure out the rest of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds better -- I reverted the script comment change so it again ends with coverage=true) in anticipation of the change. Thanks for the suggestions.

The clutter seems worth it to make code-coverage and track-allocation more painless (/ hopefully common).
@garborg garborg changed the title Add .gitignore and complete travis script for new packages Add .gitignore for new packages Jan 2, 2015
@tkelman
Copy link
Contributor

tkelman commented Jan 2, 2015

win32 appveyor failure is unrelated (@vtjnash should really work through pull requests more often to avoid breaking master), travis osx failure is Homebrew/legacy-homebrew#32819

jakebolewski added a commit that referenced this pull request Jan 4, 2015
Add .gitignore for new packages
@jakebolewski jakebolewski merged commit dafa9c8 into JuliaLang:master Jan 4, 2015
@tkelman tkelman added the packages Package management and loading label Jan 4, 2015
@tkelman
Copy link
Contributor

tkelman commented Jan 4, 2015

does coverage / track-allocation work on 0.3? if so we should label for backport pending

@jakebolewski
Copy link
Member

This could be backported

tkelman pushed a commit that referenced this pull request Jan 6, 2015
The clutter seems worth it to make code-coverage and track-allocation more painless (/ hopefully common).

(cherry picked from commit 5b643ec)
ref PR #9541

Conflicts:
	base/pkg/generate.jl
@tkelman
Copy link
Contributor

tkelman commented Jan 6, 2015

backported in 30a892b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants