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 simplecov 0.18 support #420

Conversation

deivid-rodriguez
Copy link

This is just an experiment to try to learn some go and fix #413 as a side effect.

This is not ready (I assume I would at least need to add back support for simplecov 0.17 or lower), and maybe it's not even correct πŸ€·β€β™‚οΈ, but it should be a start.

It seems to be working properly for me in deivid-rodriguez/byebug#646.

And remove simplecov 0.17 or lower support.
@PragTob
Copy link

PragTob commented Feb 10, 2020

πŸ‘‹

Not to spook in here as the old grumpy fellow (btw. πŸ‘‹ @deivid-rodriguez πŸ’š ) but imo this doesn't really fix the problems. It returns the working state to what it was before, yes.
However, I'm still rather likely to change the format again (right now we need to evil eval things which I'm not too fond off) plus the other problems with skipping, filtering and adding files described in #413 are still there.

@deivid-rodriguez
Copy link
Author

Well, the previous state worked pretty well for me, and I guess for other people too, so I think going back to the previous working state seems like a good move anyways, regardless of trying to figure out a better integration? I mean, without this PR I can no longer get diff and total coverage enforced in my PRs and use simplecov 0.18 at the same time. With this PR I can.

Things could be improved with extra stuff like the things you mentioned in #413's first post, sure! Does that justify not fixing the current integration in favour of a future better integration? Not so sure...

@PragTob
Copy link

PragTob commented Feb 11, 2020

@deivid-rodriguez I'm not saying this shouldn't be merging. I thinks it's great you did it and it's nice.

My intention was to remind people that this doesn't really "fix" #413 (as in no further work to do there) - it fixes part of the problems described and it's somewhat likely to break again (due to simplecov-ruby/simplecov#801 and whatever thing is done to adjust it).
I'd rather it not be forgotten, because then in the future the other problems persist and whenever we change the file format CodeClimate has to play catch up again πŸ€·β€β™€οΈ :

Moreover, this would probably have to be amended to also support the format < 0.18.x (or add some kind of version constraint).

@deivid-rodriguez
Copy link
Author

deivid-rodriguez commented Feb 11, 2020

@deivid-rodriguez I'm not saying this shouldn't be merging. I thinks it's great you did it and it's nice.

My intention was to remind people that this doesn't really "fix" #413 (as in no further work to do there) - it fixes part of the problems described and it's somewhat likely to break again (due to simplecov-ruby/simplecov#801 and whatever thing is done to adjust it).

I'd rather it not be forgotten, because then in the future the other problems persist and whenever we change the file format CodeClimate has to play catch up again πŸ€·β€β™€οΈ

Oh, sorry @PragTob, I misunderstood you! Yeah, I think most people is aware of that at this point, but it's good to make an explicit remainder in this PR πŸ‘.

Moreover, this would probably have to be amended to also support the format < 0.18.x (or add some kind of version constraint).

Yup, I mentioned this in the PR description, and I'm planning to amend the PR like that, indeed.

@PragTob
Copy link

PragTob commented Feb 11, 2020

Oh, sorry @PragTob, I misunderstood you! Yeah, I think most people is aware of that at this point, but it's good to make an explicit remainder in this PR +1.

No problem, I could have probably expressed my intent clearer in my initial comment. Glad we sorted this out 🀞

Also sorry for missing your initial comment on the 0.17 support.

@deivid-rodriguez
Copy link
Author

I restored simplecov 0.17 support. Now I'm working on learning some more about golang to be able to make the patch a bit more DRY and simpler.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@williamweckl
Copy link

Any updates on this one?

@elizabrock
Copy link

@deivid-rodriguez Since you added the 0.17 support, is this ready for prime time? I'm hoping that if you update the PR description and sign the contributor agreement, they'll get this merged in soon :)

@deivid-rodriguez
Copy link
Author

From what I understood, this is not going to get merged, because an alternative solution will be implemented πŸ€·β€β™‚οΈ.

@elizabrock
Copy link

elizabrock commented Jul 17, 2020

@deivid-rodriguez. My interpretation of the conversation is that the maintainer of SimpleCov (@PragTob) commented here to say that while this solution works, that it isn't future-proof. Like you, I think that we would all benefit from getting a working implementation and then proceeding from there.

I don't think that any of the maintainers of codeclimate/test-reporter (e.g. @davehenton ) have chimed in yet.

My suspicion is that they are waiting to chime in until the PR has passed all of the automated checks, one of which is now signing the contributor license agreement. So, I am hoping that if you sign the agreement and update your PR description to indicate that the work is complete, this PR will proceed along from there :)

@deivid-rodriguez
Copy link
Author

I have no idea, honestly. Last time I checked, this PR was working but I was not fully satisfied with the implementation. I'm no longer using this library, but if maintainers express their intention to merge this, I'm happy to sign the CLA, and even trying to dedicate a bit more time to it.

@deivid-rodriguez
Copy link
Author

Just to be sure, did you read #413? That's where I got my conclusions from.

@elizabrock
Copy link

Ah, I see what you are saying now, @deivid-rodriguez! That issue has progressed since the last time I read it

@PragTob
Copy link

PragTob commented Jul 18, 2020

(FWIW @deivid-rodriguez is also a simplecov maintainer πŸ˜„ πŸ’š )

@williamweckl
Copy link

Any updated on this?

@PragTob
Copy link

PragTob commented Sep 22, 2020

πŸ‘‹

I don't think it's likely that this will be adjusted and merged at this point. CodeClimate went ahead and created a JSON formatter which we're about to integrate in simplecov: simplecov-ruby/simplecov#923

That will then return a structure code climate can rely upon.

@deivid-rodriguez
Copy link
Author

Thanks for the heads up!

@deivid-rodriguez deivid-rodriguez deleted the add_simplecov_018_support branch September 23, 2020 08:20
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.

Possible Incompatibility with SimpleCov 0.18+
6 participants