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

Clean code, recursive directory copying and path normalization #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HoldYourWaffle
Copy link

@HoldYourWaffle HoldYourWaffle commented Feb 25, 2019

I found this module to be broken after 2 years without updates, so I cleaned it up a bit and fixed some path normalization.
Main changes are:

Also fixes #4 by mentioning the quote requirement in the readme.

Fixes zont#4 by mentioning the quote requirement in the readme
@HoldYourWaffle HoldYourWaffle changed the title Clean code + fix copying directories Clean code , recursive directory copying and path normalization Feb 25, 2019
@HoldYourWaffle HoldYourWaffle changed the title Clean code , recursive directory copying and path normalization Clean code, recursive directory copying and path normalization Feb 25, 2019
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated
chokidar.watch(sourceGlobs, {
ignoreInitial: true
})
.on('ready', () => sourceGlobs.forEach(s => console.log('[WATCHING]'.cyan, s)))
Copy link
Owner

@zont zont Feb 26, 2019

Choose a reason for hiding this comment

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

Why color changed?

Copy link
Author

Choose a reason for hiding this comment

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

I added some color diversity to the log messages to allow for easier reading. COPY is still yellow, I changed WATCHING to cyan, DELETE to red and added CLEAN in magenta.
I chose these colors (almost) randomly, if you want them changed just say the word.

@zont
Copy link
Owner

zont commented Feb 26, 2019

Too many fixes.

npm run lint show 71 errors

@HoldYourWaffle
Copy link
Author

Whoops I completely forgot about the linter.
I fixed almost all errors (most of them were the indentation), with just 2 remaining.
These relate to the evil global variables, and it's a bit of a chicken-and-egg problem.

ESLint wants me to declare these variables before they are used. Fair game, it would be really stupid to bury globals like this. ESLint also wants me to define functions before they are used, again this makes a lot of sense. To solve this I can declare the globals on top, after that the functions that use them and lastly the actual code. Good? Wrong: ESLint wants me to declare these globals using const because they are never reassigned.
This of course isn't possible, because I can't decalre a variable using const without immediately initializing it. The only solution I see would be to split the acting code in half, the first part (which initializes the globals) before we define our functions, and the second part (which actual does stuff) afterwards.
I feel like this would actually make the code worse instead of cleaner, but of course if you really want to I could change it.


const findTarget = from => {
/* FUNCTIONS */
function findTarget(from) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why arrow functions converted to ES5 functions?

@zont
Copy link
Owner

zont commented Feb 26, 2019

Still 2 lint errors

@HoldYourWaffle
Copy link
Author

HoldYourWaffle commented Feb 26, 2019 via email

@zont
Copy link
Owner

zont commented Feb 26, 2019

Please fix 2 lint errors

@HoldYourWaffle
Copy link
Author

As I said earlier:

ESLint wants me to declare these variables before they are used. Fair game, it would be really stupid to bury globals like this. ESLint also wants me to define functions before they are used, again this makes a lot of sense. To solve this I can declare the globals on top, after that the functions that use them and lastly the actual code. Good? Wrong: ESLint wants me to declare these globals using const because they are never reassigned.
This of course isn't possible, because I can't decalre a variable using const without immediately initializing it. The only solution I see would be to split the acting code in half, the first part (which initializes the globals) before we define our functions, and the second part (which actual does stuff) afterwards.
I feel like this would actually make the code worse instead of cleaner, but of course if you really want to I could change it.

There are 3 solutions:

  • Splitting the 'acting' code (which would be really bad)
  • Relaxing the lint settings
  • Removing the globals (which would complicate some pieces of code)

I think relaxing the lint settings would be our best option, since splitting the 'acting' code hurts readability a lot.

@HoldYourWaffle
Copy link
Author

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Folders don't appear to be mirrored TypeError: Path must be a string. Received undefined
2 participants