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

Assorted cleanup #10

Merged
merged 6 commits into from
Dec 9, 2017
Merged

Assorted cleanup #10

merged 6 commits into from
Dec 9, 2017

Conversation

waldyrious
Copy link
Contributor

I decided to re-submit #9 to prevent that work from being lost, but I started doing some cleanup in the meantime, so I decided to submit them first. That way, the doubleTurn PR will be clean.

@coveralls
Copy link

Coverage Status

Coverage decreased (-38.5%) to 61.538% when pulling 2f2708d on waldyrious:cleanup into b5a8e73 on yakir12:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-38.5%) to 61.538% when pulling 2f2708d on waldyrious:cleanup into b5a8e73 on yakir12:master.

@yakir12
Copy link
Owner

yakir12 commented Dec 8, 2017

This is fantastic, thank you!
I don't quite understand why the coverage decreased by ~40%...?

@waldyrious
Copy link
Contributor Author

Neither do I :/ the lines marked as uncovered were the ones where I changed the variable names, but I see no reason why that would make the coverage start failing...

@yakir12
Copy link
Owner

yakir12 commented Dec 8, 2017

Can you run test with coverage on your branch and see if the relevant lines are getting hit? They really should, if not, something really weird is going on..

@waldyrious
Copy link
Contributor Author

Sure, how do I do that? (Sorry, I haven't been actively developing Julia in a while...)

@yakir12
Copy link
Owner

yakir12 commented Dec 8, 2017

run:

Pkg.test("UnitfulAngles", coverage=true)

and in the source directory of the package (joinpath(Pkg.dir("UnitfulAngles"), "src")) you should fine a file with a cov extension (e.g. UnitfulAngles.jl.9683.cov). Look in it to see if those lines are being hit.

@waldyrious
Copy link
Contributor Author

Thanks for the detailed instructions :) here's the output: https://gist.github.com/waldyrious/d5ea5dfba5e1e172b4e41b79f483296c

@yakir12
Copy link
Owner

yakir12 commented Dec 9, 2017

Right. Those lines are getting tested just fine. This was some weird bug with coveralls...

@yakir12 yakir12 merged commit c3dc5cd into yakir12:master Dec 9, 2017
@waldyrious waldyrious deleted the cleanup branch December 9, 2017 12:20
@waldyrious
Copy link
Contributor Author

Thanks. I'll do the #9 re-submission PR next week.

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

3 participants