OOP school library: preserve data #6
Conversation
atok624
commented
May 23, 2023
atok624
commented
There was a problem hiding this comment.
STATUS: REQUIRED CHANGES ♻️♻️
Hi @atok624 and @shafiuyushawu
Good job so far! I know you worked hard to implement this project, I really appreciate the effort you put in! But there are some issues that you still need to work on to go to the next project but you are almost there!
To highlight 🍾
- All linters are passing ✔️
- Good commit history ✔️
- Good PR description ✔️
- Follow Gitflow ✔️
Required changes ♻️ ♻️
- Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please ping me @tresorsawasawa when you comment so I can receive the notification or use slack with the same name.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
app.rb
Outdated
| require_relative 'modules/Rental_mod' | ||
|
|
There was a problem hiding this comment.
-
Good job as your application is working as expected. However it can run at the bigening and raises a
LoadErrorbecause of the above typing error of your file name. You wroteRental_modinstead ofrental_mod. I suggest you fix it. -
[ OPTIONAL ] RECOMMENDED: I think it would be better if you use
requireinstead ofrequire_relativeto import all file in yourapp.rbfile for the following reason and best practices:requireis used to import file(s) of differentfoldersordirectoriesas it is case here andrequire_relativeis used for files of the samefolderordirectory
There was a problem hiding this comment.
Hi @atok624 and @shafiuyushawu
When I changed this require_relative 'modules/Rental_mod' to require_relative 'modules/rental_mod' you application works as expected. You can double check after correcting the error and see if you didn't include other errors by mistake
There was a problem hiding this comment.
Hi @tresorsawasawa,
We are so grateful for these suggestions. We will implement all of it.
Thank you.
There was a problem hiding this comment.
Changes Required ♻️
Hi Team,
Nice job, you are almost there.
(Highlights) Good Points: 👍
- Your work is well documented. ✔
- The correct workflow is used. ✔
- All linters checks are ok. ✔
(Changes Required) Aspects to improve: ♻️
- Please check the inline comments below ⬇️.
[Optional] suggestions:
- Nothing to mention.
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear, and please remember to tag me in your question so I can receive the notification**
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
modules/read_data.rb
Outdated
| require 'pry' | ||
|
|
There was a problem hiding this comment.
- There is an error raised here when I try to run the app, Please fix that.
as suggested in the terminal log above using pty instead of pry fixes the problem.
modules/select.rb
Outdated
| require 'pry' | ||
|
|
There was a problem hiding this comment.
- Same problem raised here. Please fix that.
There was a problem hiding this comment.
Hi @Omar-Muhamad,
We've seen the error you are talking about and we are going to work on that
Thank you.
There was a problem hiding this comment.
STATUS: APPROVED ✔️ ✔️
Hi @atok624 and @shafiuyushawu,
✔️ Implementation of data preservation
Your project is complete! There is nothing else to say other than... it's time to merge it 
Congratulations! 🎉
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear. And please, remember to tag me in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
|
Thanks so much for this review @TedLivist |
