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.

Sign up for GitHub

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

Jump to bottom

Separated out the redundant directory check code #37

Closed
wants to merge 1 commit into from
Closed

Separated out the redundant directory check code #37

wants to merge 1 commit into from

Conversation

Cazaimi
Copy link
Contributor

@Cazaimi Cazaimi commented Oct 8, 2019 •
edited
Loading

Description

In order to refactor the code, the change includes separation of the redundant directory check, which checks if config.json and teachcode-solutions already exists in the pwd, into a new function as it is a discrete operation of its own.

Fixes #16

Type of change

  • Enhancement

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [N/A] I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • [N/A] I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules
  • New and existing unit tests pass locally with my changes

@welcome
Copy link

welcome bot commented Oct 8, 2019

Thanks for opening this pull request! Please check out our contributing guidelines.

@jamesgeorge007 jamesgeorge007 self-requested a review October 8, 2019 14:10
@jamesgeorge007 jamesgeorge007 added code refactoring attempt to eliminate code redundancy Hacktoberfest labels Oct 8, 2019
jamesgeorge007
jamesgeorge007 reviewed Oct 8, 2019
View reviewed changes
Copy link
Member

@jamesgeorge007 jamesgeorge007 left a comment

Choose a reason for hiding this comment

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

It seems the current approach is pretty straight forward and I don't find the respective change as required.
Feel free to prove I'm wrong 👍

@Cazaimi
Copy link
Contributor Author

Cazaimi commented Oct 8, 2019

The separation of this from inside the initTasks function helps in readability. For example, now any person can just read the function being called, read the JSDoc and understand what's happening, without having to read the entire redundancy check code.

@jamesgeorge007 jamesgeorge007 added this to the v1.2.0 milestone Oct 8, 2019
jamesgeorge007
jamesgeorge007 requested changes Oct 8, 2019
View reviewed changes
src/commands/init.js Outdated
@@ -88,6 +80,22 @@ const initTasks = async () => {
)}`,
Copy link
Member

@jamesgeorge007 jamesgeorge007 Oct 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You could move this part out of the helper method.

src/commands/init.js Outdated Show resolved Hide resolved
src/commands/init.js Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Oct 12, 2019

There were the following issues with this Pull Request

  • Commit: 5e972c1
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: e3306fc
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@Cazaimi Cazaimi force-pushed the feature/move-redundantdircheck-to-newfunction branch from e3306fc to 0279232 Compare October 12, 2019 09:27
@ghost
Copy link

ghost commented Oct 12, 2019

There were the following issues with this Pull Request

  • Commit: 0279232
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@Cazaimi Cazaimi force-pushed the feature/move-redundantdircheck-to-newfunction branch from 0279232 to 9d5d5bb Compare October 12, 2019 14:16
@Cazaimi
Copy link
Contributor Author

Cazaimi commented Oct 12, 2019

@jamesgeorge007 , can you give me an idea as to how to pass this commit linter's tests?

@Cazaimi
Copy link
Contributor Author

Cazaimi commented Oct 12, 2019

Also, it might be much more helpful and even provide a cleaner experience if this was a part of the Git pre commit hook like husky.

@jamesgeorge007
Copy link
Member

@Cazaimi the respective update for commit message conventions was brought about by a
recent PR. Pre-commit hook was configured as well, it seems that your
branch might be outdated.

You may check out these guidelines

@madlabsinc madlabsinc deleted a comment Oct 16, 2019
@jamesgeorge007 jamesgeorge007 removed this from the v1.2.0 milestone Dec 23, 2019
@jamesgeorge007
Copy link
Member

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesgeorge007 jamesgeorge007 jamesgeorge007 requested changes

Assignees

@Cazaimi Cazaimi

Labels
code refactoring attempt to eliminate code redundancy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor the codebase to be more concise
2 participants
@Cazaimi @jamesgeorge007