-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix doctests in attiny-hal
#616
base: main
Are you sure you want to change the base?
Conversation
928daa8
to
4df103f
Compare
On Sun, Dec 29, 2024 at 05:08:53PM -0800, Iris Artin wrote:
Hey, #617 has also "Fixed attiny doctests" |
Yeah I have several PRs that are all against (I wish GH had better support for stacked PRs, but it doesn't, so here we are; I am happy to use whatever workflow works best for you.) |
On Mon, Dec 30, 2024 at 06:06:57AM -0800, Iris Artin wrote:
On Mon Dec 30 2024 Geert Stappers wrote:
> On Sun, Dec 29, 2024 at 05:08:53PM -0800, Iris Artin through github:
>
> > @innermatrix pushed 1 commit.
> > Fixed attiny doctests
>
> Hey, #617 has also "Fixed attiny doctests"
Yeah I have several PRs that are all against `main` but they are stacked
on top of each other so the later ones include all the commits from
the previous ones. If you'd like me to update them so that each is
against the topic branch of the previous one, I can do that.
Best answer to "If you'd like" is asking "How would I like it myself".
I myself would NOT like to be flooded with merge requests. I would be
annoyed when I had to review the same commit at several places.
So yes, it is a good thing to present commits
with a mindset of "My commits are a pleasure to review".
Groeten
Geert Stappers
More bystander as project member.
(Rahix is the project lead.)
--
Silence is hard to parse
|
On Mon, Dec 30, 2024 at 08:05:42AM -0800, Geert Stappers wrote:
On Mon, Dec 30, 2024 at 06:06:57AM -0800, Iris Artin wrote:
> On Mon Dec 30 2024 Geert Stappers wrote:
> > Hey, #617 has also "Fixed attiny doctests"
>
> Yeah I have several PRs that are all against `main` but they are stacked
> on top of each other so the later ones include all the commits from
> the previous ones. If you'd like me to update them so that each is
> against the topic branch of the previous one, I can do that.
Best answer to "If you'd like" is asking "How would I like it myself".
I myself would NOT like to be flooded with merge requests. I would be
annoyed when I had to review the same commit at several places.
So yes, it is a good thing to present commits
with a mindset of "My commits are a pleasure to review".
Groeten
Geert Stappers
More bystander as project member.
(Rahix is the project lead.)
What I remember, there were messages like:
@innermate: What if refactor some parts?
@stappersg: Come back with something to review.
What I can't remember to have seen is any message from Rahix
about the refactoring proposals.
@innermate: Your "giving source code back" attempt, might not be what
you expect, might not be what you hope for. But hey, you are allowed
to keep your fork.
Groeten
Geert Stappers
--
Silence is hard to parse
|
tldr: Thank you and @Rahix for all your great work; I think my contributions would be valuable to the community, so I would love to align with you and @Rahix — in whatever way you find supportive — on my proposed direction. I think a good place to start is for you two to give feedback in #605 about my proposal to have one module to describe each MCU (for example, attiny85.rs). Everything else I think we can figure out downstream. A longer version and responses to some of your specific points:
Thank you for saying that — I very much agree with that as an organizing principle. On this particular question, I have seen two different approaches, and I have used both successfully, which is why I picked one and am letting you guide me as to whether you prefer the other. (The two approaches being: 1. every stacked PR is against Unfortunately, approach 2 (which is kinder to reviewers) is not supported by GH for cross-fork PRs. The only way I could make that work is if I was able to create PRs directly in the main repo. So for now I am just going to hold off to avoid creating more burden for you; please see below on what I think is the most valuable to review right now. And I do apologize if I overwhelmed you with too many PRs and not enough information about where to focus your limited reviewing energy.
You remember correctly! I think there are two main issues we are running into here. One is that it was not clear to me whether you or @Rahix were the project lead at this time, so I wasn't sure how much involvement from @Rahix to expect; thank you for clarifying that. The other issue is that the changes I am proposing are fairly foundational, and as a result, it's not easy to present a small thing for review. (Breaking it up into smaller pieces actually ends up being more work because of the way that the different parts currently interact with each other. See below for more on this, though.
If that's how it turns out, that's okay, but I would much prefer to create something that we can agree to merge back in. As you saw, I started some additional work that improves documentation and reliability (by adding compile-only doctests for all MCUs to CI). I also have the beginnings of support for ATtiny 0-series and 1-series MCUs that I plan to submit in a future PR. Finally, I have been thinking about what it would take to run unit tests against real AVR hardware, and I plan to bring that up for discussion when I have some coherent thoughts. I think that those contributions would likely be valuable to the community, and so I would much prefer to integrate them with the great work you are already doing, instead of creating a long-term divergence. I would be happy to work with you and @Rahix on figuring out what form those contributions should take, just let me know what works best for you. My goal here is to give back to the community in whatever form is the most supportive of you two. What I ask in return is guidance about the direction you want to take with this, and ongoing open communication. To make that easier, let me ask a specific question: When you have the time, could you or @Rahix please look at my proposed structural changes in We can (and should) have a discussion later about lesser questions (naming, style, implementation details, etc), but the main thing that I am proposing to do, and that everything else I would like to contribute is built on, is that structural change: one module to describe each MCU. So that's the main question that I would like to align on early. Once again, thank you! |
( see #623 ) |
4df103f
to
45f52a9
Compare
This PR fixes all existing doctests in
attiny-hal
and addscargo test --doc
to CI forattiny-hal
. Stacked on #605.