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

Fix challenges numbering #79

Merged
merged 3 commits into from
Sep 7, 2024
Merged

Conversation

feliperodri
Copy link

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Signed-off-by: Felipe R. Monteiro <felisous@amazon.com>
@feliperodri feliperodri added the Maintenance Maintenance related issues for the challange label Sep 6, 2024
@feliperodri feliperodri requested a review from a team as a code owner September 6, 2024 20:50
Copy link

@jaisnan jaisnan left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

@carolynzech carolynzech left a comment

Choose a reason for hiding this comment

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

I find the blank page with -- to be a bit odd -- should we just wait until we have another challenge to be challenge 7? The challenges still go 6 -> 8 visually.
I like the changes to make the links to issues consistent. I was planning to write another challenge sometime next week, so I would propose that we avoid making this blank file and instead just wait for that to be challenge 7.

@feliperodri
Copy link
Author

I find the blank page with -- to be a bit odd -- should we just wait until we have another challenge to be challenge 7?

@carolynzech when I texted locally, this would lead to the wrong numbering of the challenges until we add the challenge 7 (the sequence didn't obey the numbering order when rendered). Am I missing something? If so, I can remove the blank page. If not, I'm ok with keeping it until we include a new challenge next week.

@jaisnan
Copy link

jaisnan commented Sep 6, 2024

I find the blank page with -- to be a bit odd -- should we just wait until we have another challenge to be challenge 7?

@carolynzech when I texted locally, this would lead to the wrong numbering of the challenges until we add the challenge 7 (the sequence didn't obey the numbering order when rendered). Am I missing something? If so, I can remove the blank page. If not, I'm ok with keeping it until we include a new challenge next week.

Fwiw, i'm okay with the blank page until next week as well. @carolynzech's concern is valid, but this should be fixed shortly when we add our challenge. Feel free to disagree if you have strong opinions about this.

@feliperodri Would it help to add a screenshot of what the index page looks like somewhere in the PR description?

Signed-off-by: Felipe R. Monteiro <felisous@amazon.com>
@celinval
Copy link

celinval commented Sep 6, 2024

@feliperodri you could also enable your fork to publish documentation from your fix-numbers branch so we can review how it looks like.

I find the blank page with -- to be a bit odd -- should we just wait until we have another challenge to be challenge 7? The challenges still go 6 -> 8 visually. I like the changes to make the links to issues consistent. I was planning to write another challenge sometime next week, so I would propose that we avoid making this blank file and instead just wait for that to be challenge 7.

Which blank page are we talking about?

@feliperodri
Copy link
Author

@feliperodri you could also enable your fork to publish documentation from your fix-numbers branch so we can review how it looks like.

@celinval I'll enable it. Meanwhile, here it's an imagine:
Screenshot 2024-09-06 at 5 42 49 PM

Which blank page are we talking about?

I already removed it. I added an unnecessary blank page.

carolynzech
carolynzech previously approved these changes Sep 7, 2024
@feliperodri feliperodri enabled auto-merge (squash) September 7, 2024 18:11
@feliperodri
Copy link
Author

@celinval I think I need one more approval to get it merge.

@carolynzech
Copy link

@celinval I think I need one more approval to get it merge.

Hm, two approvals should be enough--seems like the check approval action is hanging. I'm going to try revoking my approval and then re-approving to see if that triggers the check.

@carolynzech carolynzech dismissed their stale review September 7, 2024 18:18

Dismissing review to re-approve, see comment

@feliperodri feliperodri merged commit c6cecce into model-checking:main Sep 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Maintenance related issues for the challange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants