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

Add missing quote #356

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Add missing quote #356

merged 1 commit into from
Nov 7, 2024

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Nov 4, 2024

No description provided.

@maelle maelle requested a review from a team as a code owner November 4, 2024 10:23
@maelle maelle requested review from cosimameyer and SoyAndrea and removed request for cosimameyer November 4, 2024 10:23
Copy link

github-actions bot commented Nov 4, 2024

🎉 The preview is built! Check it out 👀

@maelle
Copy link
Contributor Author

maelle commented Nov 5, 2024

@SoyAndrea The missing quote made the build fail. GitHub highlights the line in read after the missing quote, so in the future we might avoid part of the misformating by looking at the file in GitHub, see screenshot below:

image

Now, dealing with JSON/YAML means we'll often fight missing commas or quotes, that's our fate 😂

@maelle
Copy link
Contributor Author

maelle commented Nov 5, 2024

Also, we should no longer merge PR if the checks are red as the checks are stable now... Should we look into adding a branch protection rule?

@SoyAndrea
Copy link
Contributor

@SoyAndrea The missing quote made the build fail. GitHub highlights the line in read after the missing quote, so in the future we might avoid part of the misformating by looking at the file in GitHub, see screenshot below:

image

Now, dealing with JSON/YAML means we'll often fight missing commas or quotes, that's our fate 😂

Thanks @maelle! You know I had to read it several times because I couldn't see the missing quote. 😂 Now that I've checked it and it's OK, can I accept the merge and fix this commit?

@SoyAndrea
Copy link
Contributor

@drmowinckels Mo, in this PR can we change the mail of the chapter that was before, with the comment you left here?

@drmowinckels
Copy link
Member

@SoyAndrea we could, but I dont want to change the email right now, as leadership are looking into why the chapter prefer using the gmail over rladies mail. If this is the best point of contact right now, we should keep the email as is and rather change it later if we can.

So I think it is ok to merge when you can @SoyAndrea :)

Thank you @maelle for the helP!

@drmowinckels
Copy link
Member

@maelle do you have the opportunity to look into the branch protection rules and make a suggestion?

@maelle
Copy link
Contributor Author

maelle commented Nov 7, 2024

I opened #357 as yes I'd like to help with this but not this week so let's not make the idea a blocker 😸

@SoyAndrea SoyAndrea merged commit 81eea68 into main Nov 7, 2024
3 checks passed
@SoyAndrea SoyAndrea deleted the maelle-patch-2 branch November 7, 2024 18:51
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.

3 participants