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(curriculum): improve tests and instructions for Expense Tracker project #54624

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

Conversation

Dario-DC
Copy link
Contributor

@Dario-DC Dario-DC commented May 2, 2024

Checklist:

Closes #52722
Closes #53135
Closes #53463
Closes #52580

@Dario-DC Dario-DC added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. new python course labels May 2, 2024
@Dario-DC Dario-DC requested a review from a team as a code owner May 2, 2024 15:40
@github-actions github-actions bot added the scope: i18n language translation/internationalization. Often combined with language type label label May 2, 2024
@Dario-DC Dario-DC added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label May 2, 2024
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 till step 20.

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 the project.
Provided that match/ case have not been introduced in the curriculum, I think this project can incorporate that in place if if..elif statements used to create the menu.

@ilenia-magoni, @fhsinchy, @Dario-DC What do you think?

@Dario-DC
Copy link
Contributor Author

Dario-DC commented May 8, 2024

I have reviewed the project. Provided that match/ case have not been introduced in the curriculum, I think this project can incorporate that in place if if..elif statements used to create the menu.

@ilenia-magoni, @fhsinchy, @Dario-DC What do you think?

This is the first time that elif is used. Personally, I'd keep the current code.

@ilenia-magoni ilenia-magoni added the status: merge conflict To be applied to PR's that have a merge conflict and need updating label May 15, 2024
@Dario-DC Dario-DC removed the status: merge conflict To be applied to PR's that have a merge conflict and need updating label May 15, 2024
Copy link
Contributor

@ilenia-magoni ilenia-magoni left a comment

Choose a reason for hiding this comment

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

Everything good, other than: I have doubts on the last step. We never introduce how to work with multiple fails, so it's kind of suddend and a bit of a whiplash to have if __name__. I would totally want to have it taught, but I would see it in a proper multifile project.

@Dario-DC
Copy link
Contributor Author

Everything good, other than: I have doubts on the last step. We never introduce how to work with multiple fails, so it's kind of suddend and a bit of a whiplash to have if __name__. I would totally want to have it taught, but I would see it in a proper multifile project.

I'm okay with removing that step from this project. The password generator is the the first project in which we import something, and probably it makes sense to not use if __name__ == '__main__' before we explain how to import a module. In my opinion, if we decide to remove it, we should remove it from the case converter, too.

What do you think? @ilenia-magoni @zairahira @fhsinchy @larymak @ihechikara

@larymak
Copy link
Contributor

larymak commented May 16, 2024

Everything good, other than: I have doubts on the last step. We never introduce how to work with multiple fails, so it's kind of suddend and a bit of a whiplash to have if __name__. I would totally want to have it taught, but I would see it in a proper multifile project.

I'm okay with removing that step from this project. The password generator is the the first project in which we import something, and probably it makes sense to not use if __name__ == '__main__' before we explain how to import a module. In my opinion, if we decide to remove it, we should remove it from the case converter, too.

What do you think? @ilenia-magoni @zairahira @fhsinchy @larymak @ihechikara

To maintain clarity and ensure a smooth learning curve, I'm in favor of removing the if __name__ == '__main__' step from both the password generator and case converter projects for now. So this will mean entirely getting rid of the respective steps and introducing a new approach right?

@Dario-DC
Copy link
Contributor Author

To maintain clarity and ensure a smooth learning curve, I'm in favor of removing the if __name__ == '__main__' step from both the password generator and case converter projects for now. So this will mean entirely getting rid of the respective steps and introducing a new approach right?

My proposal is to remove it from the expense tracker and the case converter. And keep it in the password generator, since we talk about importing modules in that project.

If we remove it, we should simply delete the step in which we create if __name__ == '__main__' and keep the function call which is currently inside it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new python course 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
4 participants