-
-
Notifications
You must be signed in to change notification settings - Fork 627
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
Issue 1341 inheritance concept #1447
Issue 1341 inheritance concept #1447
Conversation
…er/javascript into issue-1341-inheritance-concept
…oduction for inheritance
…s to errors in concept introduction and about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make the formatting change first (to all .md
files) it's easier to review too.
Dear jakewitcherThank you for contributing to the JavaScript track on Exercism! 💙
Dear Reviewer/Maintainer
Automated comment created by PR Commenter 🤖. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. A few nit-picks.
@junedev what do you think?
concepts/inheritance/about.md
Outdated
class Pet { | ||
constructor(name) { | ||
this.name = name; | ||
} | ||
} | ||
|
||
class Dog extends Pet { | ||
constructor(name, breed) { | ||
super(name); | ||
this.breed = breed; | ||
} | ||
} | ||
|
||
let dog = new Dog('Otis', 'Pug'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like inconsistent indentation. I think we use 2 spaces everywhere. After this review, I will run format on your PR and it should auto-update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format fixed it, see this commit
/format |
Sidenote, according to
Both can be solved with the same change. |
(Closing and re-opening so that the CI checks run again) |
.gitignore
Outdated
@@ -2,3 +2,4 @@ | |||
/bin/configlet | |||
/pnpm-lock.yaml | |||
/yarn.lock | |||
/.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a global gitignore file on your local machine for excluding editor/IDE related folders. We don't want this file to grow with each new editor a contributor brings along.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed from the project .gitignore and added to global .gitignore
@SleeplessByte I trust your review of the content. I double checked the general structure, looks all good (besides the missing concept entry the linter mentioned). @jakewitcher Well done overall, thanks a lot! |
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
… all inheritance related md files
I believe I have addressed all of the outstanding issues, let me know if there's anything I have missed. Thanks for all of your feedback and patience, this was my first non-trivial PR for an Exercism language track and it was really helpful for learning about the overall structure of the track code bases and the PR expectations. |
Fixes #1341