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

feat(curriculum): add Projectile Trajectory project to SciComPy #54839

Open
wants to merge 105 commits into
base: main
Choose a base branch
from

Conversation

ilenia-magoni
Copy link
Contributor

Checklist:

Closes #XXXXX

@ilenia-magoni ilenia-magoni added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels May 20, 2024
@zairahira zairahira self-assigned this May 20, 2024
Copy link
Contributor

@Dario-DC Dario-DC left a comment

Choose a reason for hiding this comment

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

Once the tests are fixed, instructions can be refined a bit for the last steps but it's looking very good!

Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

Reviewed the project and left a few comments.
Also, I wasn't able to pass the steps 18, 19 and 20.

Some steps(like step-2) are too wordy and would need to be revised.

Once Dario's reviews are resolved, I'll take another thorough look.

Copy link
Contributor

@fhsinchy fhsinchy left a comment

Choose a reason for hiding this comment

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

I went through the steps. Looks fine to me. I managed to pass all the steps. The text looks great too.

@@ -874,6 +874,10 @@
"This is one of the required projects to claim your certification."
]
},
"learn-encapsulation-by-building-a-projectile-trajectory-calculator": {
"title": "Learn Encapsulation by Building a Projectile Trajectory Calculator",
"intro": ["", ""]
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to add intro.

"usesMultifileEditor": true,
"hasEditableBoundaries": true,
"dashedName": "learn-encapsulation-by-building-a-projectile-trajectory-calculator",
"order": 221,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to check the order.

Co-authored-by: Zaira <33151350+zairahira@users.noreply.github.com>
Copy link
Member

@zairahira zairahira left a comment

Choose a reason for hiding this comment

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

I have reviewed all the steps, the tests ran fine for all of them.
I have left a couple of comments, after that the project is ready to go from my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs to be fleshed out, see #43289 (comment)

print(ball)
coordinates = ball.calculate_all_coordinates()
graph = Graph(coordinates)
print(graph.create_trajectory())
Copy link
Member

Choose a reason for hiding this comment

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

This isn't very readable in next steps, which might make it harder to get the correct results.

Maybe another step could be added to change it for couple steps?:

for row in graph.create_trajectory()
    print(row)

Comment on lines 12 to 23
```py
[
"⊣ ∙ ",
"⊣ ∙∙∙ ∙∙∙ ",
"⊣ ∙ ∙ ",
"⊣∙ ∙ ",
"⊣ ∙ ",
"⊣ ∙",
"",
" ⊤⊤⊤⊤⊤⊤⊤⊤⊤⊤⊤⊤⊤",
]
```
Copy link
Member

Choose a reason for hiding this comment

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

Is it just me, or this is not fully looking like it should?

Are the special characters messing up mono-spacing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are messing up the mono-spacing, let me see if I find a different character...

Comment on lines +31 to +39
The method `create_trajectory` should return `graph`.

```js
({
test: () => runPython(`
assert _Node(_code).find_class('Graph').find_function('create_trajectory').has_return('graph'), "return graph missing"
`)
})
```
Copy link
Member

Choose a reason for hiding this comment

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

If the exact value from function is tested (next hint) I think it's fine to not check how the returned thing is named in function. This likely apply to couple other steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new python course platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants