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

Axis aware #25

Merged
merged 11 commits into from
Apr 11, 2020
Merged

Axis aware #25

merged 11 commits into from
Apr 11, 2020

Conversation

briochemc
Copy link
Collaborator

This PR is so that we can check that the docs deploy nicely before merging.

There will probably be a bit of more work required to fix the Travis failures that I have ignored so far, too :)

@codecov-io
Copy link

codecov-io commented Apr 7, 2020

Codecov Report

Merging #25 into master will decrease coverage by 12.08%.
The diff coverage is 75.51%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #25       +/-   ##
===========================================
- Coverage   84.14%   72.05%   -12.09%     
===========================================
  Files           1        1               
  Lines          82       68       -14     
===========================================
- Hits           69       49       -20     
- Misses         13       19        +6     
Impacted Files Coverage Δ
src/UnitfulRecipes.jl 72.05% <75.51%> (-12.09%) ⬇️

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 24dbe88...45ff062. Read the comment docs.

@briochemc
Copy link
Collaborator Author

One solution is to ignore Julia v1.0 🙈

@jw3126
Copy link
Owner

jw3126 commented Apr 7, 2020

I am fine with ignoring 1.0.

strategy:
matrix:
version:
- "1.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Let see what is the minimal julia version that passes CI

Suggested change
- "1.0"
- "1.0"
- "1.1"
- "1.2"
- "1.3"
- "1.4"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMHO we don't need the other julia versions, i.e., CI should restrict itself to only

  • 1.0 (long-term-support version, currently v1.0.5)
  • 1 (latest stable release, currently v1.4.0)
  • nightly (development branch)

So below I just removed 1.0 because it segfaults for some reason I don't understand (and my attempts to analyze and report it following this have failed so far)

@briochemc
Copy link
Collaborator Author

BTW if you are OK with it, I added the GitHub actions for CI to replace the Travis script. Could you do the things I pointed out in #22? (I can't do these myself, it needs to be the repo owner)

@briochemc briochemc requested a review from jw3126 April 9, 2020 01:08
@briochemc
Copy link
Collaborator Author

briochemc commented Apr 10, 2020

This looks much better than the old implementation. Thanks a lot! Does it get the axes right for histogram?

Oh actually it does not work for histogram... [edited ->] Will point this out onto somehow to Plots.jl / RecipesBase.jl peeps Started a Plots.jl issue about this

@jw3126
Copy link
Owner

jw3126 commented Apr 10, 2020

BTW if you are OK with it, I added the GitHub actions for CI to replace the Travis script. Could you do the things I pointed out in #22? (I can't do these myself, it needs to be the repo owner)

Thanks for all of your outstanding work. And sorry that I could not take care of the CI secrets yet and for my slow reviews. Corona converted almost all of my hobby programming time into child care time.

Copy link
Owner

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

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

At first glace this looks good. Thanks a lot! I think travis (and appveyor) should be deleted or alternatively allowed to fail if you think it contains useful information. Can you remind me how to preview the docs?

@jw3126 jw3126 mentioned this pull request Apr 10, 2020
@briochemc
Copy link
Collaborator Author

Thanks for all of your outstanding work. And sorry that I could not take care of the CI secrets yet and for my slow reviews. Corona converted almost all of my hobby programming time into child care time.

No worries!

@briochemc
Copy link
Collaborator Author

At first glace this looks good. Thanks a lot! I think travis (and appveyor) should be deleted or alternatively allowed to fail if you think it contains useful information. Can you remind me how to preview the docs?

Removed the .travis.yml file in commit 4a3ce10, for which we should see the preview docs from now on I think 🙂

@briochemc
Copy link
Collaborator Author

Well it looks like the PR did not make it into gh-pages... Could you make sure that you re-generate the documenter keys (instructions here) and put the private one in your "secrets" and the replace the public one? And then I can retrigger the docs' GitHub action...

@jw3126
Copy link
Owner

jw3126 commented Apr 10, 2020

I think I did everything correctly, but I can redo now just in case.

@jw3126
Copy link
Owner

jw3126 commented Apr 10, 2020

I regenerated them. Maybe I forgot to give write access before.

@briochemc
Copy link
Collaborator Author

OK I'll add this new image for the readme just to trigger CI (and make it look a bit nicer?)

UnitfulRecipeExample

@briochemc
Copy link
Collaborator Author

OK it worked, here is the preview: https://jw3126.github.io/UnitfulRecipes.jl/previews/PR25/

@briochemc
Copy link
Collaborator Author

Is it OK for me to merge and tag a release?

Copy link
Owner

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

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

LGTM feel free to merge and tag a release. And thanks again for this great PR. Code is much cleaner now.

@daschw
Copy link

daschw commented Apr 10, 2020

I don't want to promise too much but the segfault on julia 1.0 could be caused by a faulty precompiles file in Plots and might be fixed with the next Plots release.

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.

None yet

4 participants