rstudio / pins-python

https://rstudio.github.io/pins-python/
MIT License
50 stars 12 forks source link

Implement `force_identicial_write` #260

Closed nathanjmcdougall closed 2 weeks ago

nathanjmcdougall commented 2 months ago

To resolve #241.

I struggled for a while to realize that prepare_pin_version deletes the old pin via version_setup for unversioned boards. To deal with this, I have pre-emptively retrieved the meta object before it is deleted, and then used that to compare the hash.

I think part of the issue is perhaps that version_setup doesn't belong in pins/versions.py - I am of the view that it is currently too decontextualized and would be better closer to the place where it is invoked, in prepare_pin_version. A docstring would help too.

It's worth noting that this is a breaking change - as a result some of the existing tests needed modifying to opt-in to the old behaviour.

I'm not sure whether the READMEs need any updates. I took a read but I think it would be distracting to detail this feature.

nathanjmcdougall commented 2 months ago

I've realized the best place to add docs is probably the get_started.qmd notebook.

However, it's hard to demonstrate the functionality without encountering the same issue as described in #258.

Perhaps the solution is created= arguments, but this muddies the illustration. Another option is to separate the calls between separate cells and hope the user doesn't do Run Above or a similar high-frequency cell execution.

For now, I have used time.sleep.

isabelizimm commented 2 months ago

just wanted to say that I've seen this and will give it a closer look in the next ~few days~ (edit: got swept away with positconf prep, but will be getting back into the swing of things here shortly)! and to say I've never seen pytest.CaptureFixture[str] before and am VERY EXCITED to learn about its existence 👀

nathanjmcdougall commented 1 month ago

I hope the conference went well!

I think the test that's failing needs to change based on the new default behaviour, I'll take a look now.

isabelizimm commented 1 month ago

It did! It's one of my favorites (not that I am biased or anything 😆)

I think the test that's failing needs to change

++ I can kick off the full set of tests again in a moment here to pick up your new commits!

isabelizimm commented 1 month ago

I struggled for a while to realize that prepare_pin_version deletes the old pin via version_setup for unversioned boards.

Late comment, but some history here is that Posit Connect cannot delete the most recent pin. IIRC that is in place since pins actually has to create a second pin and then delete the older pin for an unversioned board. The placement is also in parity with the R side; that being said, parity is not something hugely important to maintain if it doesn't make sense but is a subtle factor sometimes. I plan to leave it as is for now, but it is good to note the internal ergonomics! 💯