search

mrkaye97 / slackr

An R package for sending messages from R to Slack
https://matthewrkaye.com/slackr/
Other
307 stars 83 forks source link

fix duration in slackr_history if posted_from_time is not provided #182

Closed czeildi closed 1 year ago

czeildi commented 1 year ago

Fixes #181

czeildi commented 1 year ago

I am not sure if I should also include a version bump and news item with this tiny change

mrkaye97 commented 1 year ago

I am not sure if I should also include a version bump and news item with this tiny change

Yes, please put in a patch version bump and add a note to the changelog! Thanks!

czeildi commented 1 year ago

I removed methods from Description file which caused a Note in R cmd check and bumped the roxygen version number so that r cmd check does not complain about mismatch, but the test failures are not related to this PR, I believe. Many of them complain about slack not authed, maybe the slack token set up in github actions is expired?

mrkaye97 commented 1 year ago

I removed methods from Description file which caused a Note in R cmd check and bumped the roxygen version number so that r cmd check does not complain about mismatch, but the test failures are not related to this PR, I believe. Many of them complain about slack not authed, maybe the slack token set up in github actions is expired?

Ah, it's actually because I don't think GH likes sharing its secrets to a fork. Let me look into this for a bit and see if I can set up a better way of managing secrets so that I can share the token your CI runs need. I'll get back to you on this in a bit

mrkaye97 commented 1 year ago

alright @czeildi, do you mind trying to rebase your branch on top of the latest upstream master? I just changed some things about how the CI works and fixed a couple failing tests, so I think (75% chance) that if you rebase and I approve your CI run, all the tests should pass 🤞. and thanks for putting up with the toil here! hopefully the CI process is a little smoother going forward...

czeildi commented 1 year ago

alright @czeildi, do you mind trying to rebase your branch on top of the latest upstream master? I just changed some things about how the CI works and fixed a couple failing tests, so I think (75% chance) that if you rebase and I approve your CI run, all the tests should pass crossed_fingers. and thanks for putting up with the toil here! hopefully the CI process is a little smoother going forward...

Thank you for the help! The sync button for forks created a merge commit instead or rebasing, is that a problem?

mrkaye97 commented 1 year ago

Merge commit should be fine! Looks like my fix didn't work though :/

The issue is that the secrets I have set up aren't getting injected into your CI environment correctly. In an effort to not stall this PR for forever, I'm going to try merging from your fork into my own branch of the same name, and running CI there. I think it'll all pass once that happens 🤦