search

sinanpl / OaxacaBlinder

R implementation of Oaxaca-Blinder gap decomposition
MIT License
1 stars 1 forks source link

Add viewpoint group arg #50

Open davidskalinder opened 4 months ago

davidskalinder commented 4 months ago

Like #49, my plan is to use this branch as a private mod until #18 is sorted. This branch expands the tests in #49 to test cases where the group variable is a factor or where the viewpoint group is unspecified, and adds the actual implementation (and docs). All tests passing on my laptop, so fingers crossed that'll be the case when they run for this PR as well.

@sinanpl, as with #49, feel free to use, accept, or reject whatever's useful here.

davidskalinder commented 4 months ago

Hah, all the tests pass but the style workflow doesn't? I don't really understand that, but I'll try restyling on my end and resubmitting to see if that helps...

davidskalinder commented 4 months ago

Hah, all the tests pass but the style workflow doesn't? I don't really understand that, but I'll try restyling on my end and resubmitting to see if that helps...

Okay looks good now. My guess is this happened because the workflows don't currently run when I push (since my default master branch doesn't have the workflows yet). Instead, they run when I submit a PR; but when the styler workflow runs on a PR and finds a change, it can't commit it because a PR isn't a branch. Not a big deal at all (since the workaround of just running styler locally is super easy), but we might want to bear that in mind as a reason why workflow runs might fail.

sinanpl commented 4 months ago

Reason it fails is that I combined the styling (only at push) and test checks (push or PR).

The styling set-up works for pushes, not for PRs if there is styling needed. With most of the PRs being closed, I'm inclined to introduce precommit for R which'll format the package locally at commit, not at push / pr. I'll open an issue / PR. The styling worfklow can still check, but not do any commits that restyle the code.

Wrt to the actual content of the PR: I'll be revisiting this later :)

davidskalinder commented 4 months ago

The styling set-up works for pushes, not for PRs if there is styling needed.

Yes, that does make sense. BTW I think the whole styling trigger is even less of a problem because the only reason it didn't work as expected is because my master branch is out of date. IIUC, any forks created since the workflow implementation happened will just have styling corrections happen on push before any PRs happened. I haven't gotten around to getting the workflows into my default branch yet, which is the only reason why they had to wait until the PR.

All that said, no reason not to smooth the process out even further if it's not too onerous of course.

sinanpl commented 4 months ago

Just merged #51 into dev. This is the right way to approach styling. Please check out how to set this up (references in end of README). This will disallow you to commit anything that is bad formatting and bunch of other things.