search

metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

Add nmrec dependency #606

Closed seth127 closed 9 months ago

seth127 commented 10 months ago

Work in progress... adds nmrec as a dependency and refactors some internal functions to use it, in place of legacy code that primarily relied on regex.

seth127 commented 10 months ago

@kyleam do you have an opinion about whether it would be easier/smoother to merge this and make a bbr release before or after nmrec gets added to MPN?

In other words:

kyleam commented 10 months ago

is it easier to wait until after that snapshot?

Unless you think there's any urgency for this rework to land on MPN (I'm not aware of any), I'd prefer to wait until nmrec makes it to snapshot. We've got a good number of moving parts already with the snapshot and mrg releases, and waiting would let us finalize things (e.g., the drone config) without requiring post-release followups.

seth127 commented 10 months ago

Ok that makes sense to me @kyleam . Let's plan to revisit this in early October, once that MPN snapshot is out.

kyleam commented 10 months ago

The snapshot is out; I've adjusted the Drone config to drop the custom nmrec installation in all spots aside from the oldest build, which still requires it (2e1edb91).

https://github-drone.metrumrg.com/metrumresearchgroup/bbr/3549

seth127 commented 9 months ago

@kyleam @barrettk what's the status of this branch at this point? I know it was serving as a dev branch for awhile... are there still pending PRs coming into here, or should we go ahead and merge it?

Related, @barrettk is actively working on #582 , so we could until that is done to merge all of this. That will likely be a little while though.

I'm fine with whatever y'all want to do, I just wanted to make sure this wasn't just loitering at this point. Thanks.

kyleam commented 9 months ago

@seth127, once nmrec hit MPN, I pushed an update for the CI (2e1edb9), so it's ready to merge in my view. We no longer need a branch to merge nmrec PRs now that that nmrec is on MPN.

(Note that there's the unmerged gh-605, but my opinion is that that can be closed without merge.)

Related, @barrettk is actively working on #582 [...]

I don't think that we should wait for that to come in.

I'm fine with whatever y'all want to do, I just wanted to make sure this wasn't just loitering at this point.

As the opener of the PR, I was expecting you to switch it to "ready for review" and guide it to completion (probably just requesting a review and hitting merge at this point, though things may pop up). Would you like someone to take over that?

seth127 commented 9 months ago

Thanks for the updates.

As the opener of the PR, I was expecting you to switch it to "ready for review" and guide it to completion (probably just requesting a review and hitting merge at this point, though things may pop up). Would you like someone to take over that?

Yes, fair point. Although I would be happy if you don't mind taking it over @kyleam . I think you've reviewed most (all?) of the PRs that came into this branch, so hopefully it will be pretty quick look this one over. Let me know if I'm missing any nuance there and we can discuss a different plan, if needed.

kyleam commented 9 months ago

@barrettk If all looks good to you, please do the honors of merging to main.

barrettk commented 9 months ago

At least on my end, GitHub isn't showing the Drone builds as completed. Here they are (both green)

Yeah that is the same on my end as well. Won't let me merge until that goes through: image

@seth127 is this something we have to file a ticket for (drone hanging) like last time? It seems a bit different this time since it did actually run

kyleam commented 9 months ago

The good old off/on approach didn't work to get the status checks to show up. (Given it's not urgent to get this in, I'm okay holding this up to see if we can resolve it; however, I'm also fine with admin-bypassing this and waiting to see whether it comes up again.)

seth127 commented 9 months ago

Per this comment above everything is good, we just have a glitch in the Drone build showing up. I am merging with my admin privileges to avoid having to dig any deeper into the Drone issue. We'll dig deeper on it if this keeps happening on other PRs.