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 Project.toml for new Julia Registrator package flow #1277

Merged
merged 2 commits into from
Apr 22, 2019

Conversation

non-Jedi
Copy link
Contributor

Since packages new register directly to the General repo, a
Project.toml will be needed before Gadfly's next release.

@codecov-io
Copy link

codecov-io commented Apr 17, 2019

Codecov Report

Merging #1277 into master will decrease coverage by 4.24%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1277      +/-   ##
==========================================
- Coverage   90.08%   85.83%   -4.25%     
==========================================
  Files          36       36              
  Lines        3953     4159     +206     
==========================================
+ Hits         3561     3570       +9     
- Misses        392      589     +197
Impacted Files Coverage Δ
src/geom/beeswarm.jl 78.75% <0%> (-13.9%) ⬇️
src/ticks.jl 80.57% <0%> (-11.23%) ⬇️
src/bincount.jl 84.53% <0%> (-10.82%) ⬇️
src/geom/rectbin.jl 77.77% <0%> (-9.73%) ⬇️
src/poetry.jl 88.88% <0%> (-7.12%) ⬇️
src/geom/label.jl 92.52% <0%> (-6.48%) ⬇️
src/statistics.jl 90.81% <0%> (-6.03%) ⬇️
src/geom/subplot.jl 90.44% <0%> (-4.86%) ⬇️
src/dataframes.jl 85.71% <0%> (-4.86%) ⬇️
src/scale.jl 93.38% <0%> (-4.03%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f577e3c...a4aa42a. Read the comment docs.

@bjarthur
Copy link
Member

are we supposed to delete REQUIRE too?

isn't there supposed to be a version number in Project.toml? cf 1, 2, 3, 4, 5

did you use this tool to generate it? or this one?

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Apr 18, 2019

  • REQUIRE can certainly be deleted (but doesn't need to be); you're right.
  • I did not add a version number because I'm not sure if next release will be 1.1.0 or 1.0.1. One will be needed before next release.
  • I don't think I used a tool. This is the Project.toml that's been sitting around on my computer for the past few months for practical reasons in the Gadfly repository; pretty sure I just generated it by hand so that I could have a clean environment for Gadfly development.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Apr 18, 2019

The other thing we should talk about if whether we want to start
following the "best practice" of doing some rudimentary dependency
pinning (as is the default for the compat section of
Project.toml). Since Gadfly is post-1.0 now it seems like it shouldn't
just be assumed that either we're compatible with all versions of a
package or that we're compatible with all versions greater than some
number. Should we cap version numbers to current minor version for
pre-1.0 packages and current major version for post? e.g.

[compat]
Colors = "0.3.4, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9"

@bjarthur
Copy link
Member

bjarthur commented Apr 19, 2019

there are new features, not just bug fixes, so let's go with 1.1.

should add test/Project.toml too and delete test/REQUIRE

version pinning seem reasonable. this script might do that. would be good to compare it's output with yours anyway.

@non-Jedi
Copy link
Contributor Author

That script generated a file identical to mine with the exception that
it has a different uuid for the Statistics package; which makes me
wonder if mine is correct. Could you run the tests using the
Project.toml from this PR and confirm that it works for you?

cd("Gadfly.jl")
import Pkg
Pkg.activate(".")
Pkg.instantiate()
Pkg.test()

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Apr 19, 2019

I've moved test/REQUIRE into the Project.toml file under [targets] and [extras] as described in the Pkg.jl manual. I also added Julia 0.7 and Julia 1.1 to the CI tests since we're claiming to support them. Versions have been pinned (manually; the script did not pin versions). I think this is ready to go once CI passes. The Statistics uuid seems to be correct since CI found it without any problems.

@bjarthur
Copy link
Member

looks great. two things:

could you please squash your first two commits so as not to clutter the git log.

your uuid for Statistics matches mine and the one in the julia stdlib. would be good though to get a firm answer on why @StefanKarpinski 's script is returning a different one. perhaps submit an issue to Pkg.jl?

@non-Jedi
Copy link
Contributor Author

Done. Statistics uuid issue was corrected since I downloaded the script: JuliaLang/Pkg.jl#1138.

@bjarthur bjarthur merged commit 9f5f621 into GiovineItalia:master Apr 22, 2019
@bjarthur bjarthur mentioned this pull request Apr 23, 2019
6 tasks
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