Skip to content

Conversation

@sstevens2
Copy link
Contributor

@sstevens2 sstevens2 commented Aug 27, 2025

This was a pretty big change and I moved some stuff to callouts and spoilers. I also wrote the code and it probably isn't as elegant as it could be.

This PR still needs a test function (or 2) for the challenge before it can be merged. I'm particularly inexperienced at testing and I would love a recommendation for this. I may also ask the instructor teaching testing for ideas when he is done teaching today, if he has a moment.
@anenadic and @amangoel185 you are welcome to start reviewing.

fixes #266

We will also need to update the final state of the code at some point in the code states repo.

@github-actions
Copy link

github-actions bot commented Aug 27, 2025

🆗 Pre-flight checks passed 😃

This pull request has been checked and contains no modified workflow files or spoofing.

Results of any additional workflows will appear here when they are done.

github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
@sstevens2
Copy link
Contributor Author

After talking to the instructor, I think testing this might be a big complex. I think maybe I'll instead add a bug, where the code doesn't split by crew member at first (which my current code does do) and then have them report the bug and fix it.

github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
Copy link
Member

@tobyhodges tobyhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting a few minor changes but otherwise this looks great @sstevens2

Comment on lines 234 to 236
A feature request should include a short title, the key features of the new feature, and a more detailed description of the feature.
- Title: Add summary table of eva time by astronaut
- Description: A summary table split by astronaut would be helpful for individual level analysis.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A feature request should include a short title, the key features of the new feature, and a more detailed description of the feature.
- Title: Add summary table of eva time by astronaut
- Description: A summary table split by astronaut would be helpful for individual level analysis.
A feature request should include a short title, the key characterics or desired behaviour of the new feature, and a more detailed description of the feature.
- Title: Add summary table of eva time by astronaut
- Description: A summary table split by astronaut would be helpful for individual level analysis.

@tobyhodges
Copy link
Member

After talking to the instructor, I think testing this might be a big complex. I think maybe I'll instead add a bug, where the code doesn't split by crew member at first (which my current code does do) and then have them report the bug and fix it.

Fair enough. Thinking about this, I suggest we also include a sentence or two to address how, in reality, the original feature request issue might be reopened when the bug is reported, rather than a separate issue being opened. I think it's fine to acknowledge that and stick with the example, though. Maybe something like the following:

In practice, a report of incorrect implementation of a new feature might be made as a comment on the original feature request issue rather than via a new issue. The project maintainers might then reopen that issue until the correction is made. For the purposes of this workshop, we want you to get more practice by opening a separate issue.

Copy link
Collaborator

@anenadic anenadic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving this so we can quickly merge but I have not tested the code myself. I will try to do that after we have the material in main.

A reminder to also fix the code on the software project repo or to open an issue so we do not forget.

@anenadic anenadic merged commit 55b85af into main Aug 27, 2025
3 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
Auto-generated via `{sandpaper}`
Source  : 55b85af
Branch  : main
Author  : Aleksandra Nenadic <[email protected]>
Time    : 2025-08-27 18:20:40 +0000
Message : Merge pull request #279 from carpentries-incubator/issue266

first draft of changing the issue/pr section
github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
Auto-generated via `{sandpaper}`
Source  : 532ca82
Branch  : md-outputs
Author  : GitHub Actions <[email protected]>
Time    : 2025-08-27 18:21:18 +0000
Message : markdown source builds

Auto-generated via `{sandpaper}`
Source  : 55b85af
Branch  : main
Author  : Aleksandra Nenadic <[email protected]>
Time    : 2025-08-27 18:20:40 +0000
Message : Merge pull request #279 from carpentries-incubator/issue266

first draft of changing the issue/pr section
@anenadic
Copy link
Collaborator

Oops I have not seen Toby's comments before merging - will try to work still on this branch to make sure we have it all. Or if @sstevens2 has a look at this - she might be better positioned to address Toby's comments.

anenadic added a commit that referenced this pull request Aug 27, 2025
anenadic added a commit that referenced this pull request Aug 27, 2025
…nges

Replayed commit by @tobyhodges from PR #279 that were not merged by m…
github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
Auto-generated via `{sandpaper}`
Source  : eb9cbaf
Branch  : main
Author  : Aleksandra Nenadic <[email protected]>
Time    : 2025-08-27 18:54:53 +0000
Message : Merge pull request #281 from carpentries-incubator/issue266-tobys-changes

Replayed commit by @tobyhodges from PR #279 that were not merged by m…
github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
Auto-generated via `{sandpaper}`
Source  : 47cf474
Branch  : md-outputs
Author  : GitHub Actions <[email protected]>
Time    : 2025-08-27 18:55:30 +0000
Message : markdown source builds

Auto-generated via `{sandpaper}`
Source  : eb9cbaf
Branch  : main
Author  : Aleksandra Nenadic <[email protected]>
Time    : 2025-08-27 18:54:53 +0000
Message : Merge pull request #281 from carpentries-incubator/issue266-tobys-changes

Replayed commit by @tobyhodges from PR #279 that were not merged by m…
sstevens2 added a commit that referenced this pull request Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pull request example needs to be replaced

4 participants