rstudio / pins-python

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

Moving board_deparse to boards.py #247

Closed nathanjmcdougall closed 2 months ago

nathanjmcdougall commented 2 months ago

This helps avoid a circular input, as mentioned in this comment: https://github.com/rstudio/pins-python/blob/41c37aea979d3f09e7c0ef4d8cae40d491b02440/pins/boards.py#L1092

  1. I have kept board_deparse imported in pins.constructors to avoid breaking backward compatibility.
  2. The comment recommended moving the function to pins.utils but this would not solve the issue of a circular import, since the issue is that the function uses BaseBoard in its type annotation from pins.boards. Instead, I have placed the function in pins.boards, which I think is sensible since the function accepts board as its only arg.
  3. I am not sure whether you value PRs like this. I see TODOs in the code and I assume it is appreciated to resolve them, but I am aware that they are fairly cosmetic in cases like this, and there are trade-offs when changing the API etc.
isabelizimm commented 2 months ago
  1. I am not sure whether you value PRs like this. I see TODOs in the code and I assume it is appreciated to resolve them, but I am aware that they are fairly cosmetic in cases like this, and there are trade-offs when changing the API etc.

This is a great question. Something to keep in mind is that the TODOs are often quick notes not scoped/triaged in any particular way. Some, like this one, are more well defined to implement but there are others that might be more philosophical or a huge effort. The best way to guarantee updates are still needed are to look at the open issues. I'll go ahead and label a few with help-wanted to make that distinction a little clearer. But if there are certain TODOs that look interesting to you that aren't reflected as issues, feel free to ask and I can give some clarity!

nathanjmcdougall commented 2 months ago

Okay, that sounds good, I will make sure an issue is opened for a TODO before working on it.