-
-
Notifications
You must be signed in to change notification settings - Fork 9
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: introduce parser and cli #5
Conversation
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.
Looks good from here, great job man 👍. @jonaslagoni you might wanna review this PR
@NektariosFifes whenever the PR is ready to be reviewed you can select reviewers to the right side of this PR which sends a request to me that I should take a look at it. This is great in some cases where last-minute changes are still needed before it is getting reviewed, otherwise you can always create the PR as a draft which marks the PR as in progress. Is it ready to be reviewed? 😄 |
nope |
@jonaslagoni if you want u can review pr |
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.
So left quite a few comments in multiple locations, but generally, it looks good 👍 🙂
A few general comments:
- Why have you created the
Async-Simulator
folder? 🤔 I think we can unwrap this right? - Any files that are not used at the moment, this includes empty files, UI library files (if they are not used in the CI at the moment, should be removed from this PR. Feel free to save them on another branch 🙂
- Keep referring to the library as Fluffy Robot, at least until another name is found, this is to ensure consistency when referenced
Let me know if you have any questions about my comments, and ensure you evaluate my comments and not just blindly accept them as you might have a different opinion or know something I dont 🙂
ok i think i fixed them :) @jonaslagoni |
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.
Looks good mate ! Few changes requested, have a look it. 🙂
Our CI system requires you to name the title of the PR something like |
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.
Got some more comments 👍
Almost there 🎉
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.
Almost there 🎉
Only a final touchup, please go through all the dependencies and remove any that are unused 🙂
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.
Make sure you update package-lock.json
and commit it after the last dependencies have been assessed 🙂
@NektariosFifes you have two lint errors that have to be dealt with 🙂 The security lint warning can be disabled I think 🤔 |
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.
Last suggestion and lets merge 👍
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.
LGTM, great job! 👍
🎉 This PR is included in version 0.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Related issue(s)
#18