Skip to content
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

Update 24-diagnosing-issues-improving-robustness.md #376

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

jreeve-nv
Copy link
Contributor

Corrected error where the automating pylint section creates a branch named pylint-ci but then merges test-suite back into develop. Now the merge back into develop is with the pylint-ci branch.

Corrected error where the automating pylint section creates a branch named `pylint-ci` but then merges `test-suite` back into `develop`. Now the merge back into `develop` is with the `pylint-ci` branch.
@douglowe douglowe self-requested a review August 29, 2024 10:35
@bielsnohr
Copy link
Collaborator

Likely related to #365

@steve-crouch
Copy link
Collaborator

Also related to #383, which appears to have a different solution to the same issue if I understand it correctly. After discussing it with Aleks, we'll need to work out the best way forward after the material has transitioned to the new format which has become a priority. We'll recreate the PR after that. At least we're not short on solutions for this :)

@bielsnohr
Copy link
Collaborator

Thanks for the contribution @jreeve-nv and comments @steve-crouch

As I detailed in this comment, this is actually solved if we just get rid of creation and mention of the pylint-ci branch and continue working on the test-suite branch that learners should already be on at this point in the lesson. I have done a search through the lesson material, and there is no other reference to pylint-ci anywhere else in the lesson, so this should be a safe change with no knock-on effects.

@jreeve-nv I have pushed updates to your PR to reflect these comments. Will wait for another member of the maintainer team to approve before we go ahead and merge.

@anenadic
Copy link
Collaborator

Thanks for the contribution @jreeve-nv and comments @steve-crouch

As I detailed in this comment, this is actually solved if we just get rid of creation and mention of the pylint-ci branch and continue working on the test-suite branch that learners should already be on at this point in the lesson. I have done a search through the lesson material, and there is no other reference to pylint-ci anywhere else in the lesson, so this should be a safe change with no knock-on effects.

@jreeve-nv I have pushed updates to your PR to reflect these comments. Will wait for another member of the maintainer team to approve before we go ahead and merge.

Let's have a chat about this @bielsnohr next time we meet later this week - I am actually minded to have this work on a separate branch pylint-ci and then merge that branch onto develop branch at the end of the episode to reinforce the feature-branch development workflow.

Copy link
Collaborator

@steve-crouch steve-crouch left a comment

Choose a reason for hiding this comment

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

Just tested this suggested change to the branch instructions and it all works as expected.

@bielsnohr bielsnohr merged commit 8cc01e5 into carpentries-incubator:gh-pages Oct 25, 2024
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.

4 participants