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

Less spam in logs & less writing to disk #22

Merged
merged 1 commit into from
Mar 23, 2019

Conversation

johny-gog
Copy link
Contributor

Less spam in logs.
Write to disk only if stripped, or another destintion.

@nuzzio
Copy link
Owner

nuzzio commented Jan 10, 2019

My apologies, I had not seen this pull request. Thank you for submitting it. I will take a look at it this week.

@johny-gog
Copy link
Contributor Author

No problem, it's just something that was bothering me.

@nuzzio
Copy link
Owner

nuzzio commented Mar 15, 2019

Hi @johny-gog, finally getting some time for this. Would you be able to split verbose flag implementation and the disk writing optimization into two pull requests? I think the verbose flag is ready to go in.

As far as optimizing the writing to disk, I feel that there are use cases created by the ambiguity of "file existence = stripped file" that are problematic. It seems that if I process my files and then add a small change to the strip rules, the existence of any previously written files will prevent the application of the new rule.

@johny-gog
Copy link
Contributor Author

johny-gog commented Mar 23, 2019

Hi @nuzzio , thx for reply

I didn't introduce anything like:

checking if file exists -> if it exists, that means it was stripped already

What I did there is changed from:

Replace things in this file, or not replace if nothing to replace -> write the file to disk

to:

Replace things in this file -> Was there anything to replace in the file?
Yes -> write to disk
No -> don't write to disk, unless destination is different than the source file - then always write

I feel like most of the files do not have any code to strip. But maybe always writing to disk is not bad - I am not insisting and I don't love that change. And it would be an optimization only if someone replaces the file without a separate destination folder.

Feel free to comment again and reject this part, I'll leave the logs change only, since this was bothering me.

@nuzzio
Copy link
Owner

nuzzio commented Mar 23, 2019

Hi @johny-gog, I see what you mean. I think I misunderstood the intention of the code there.

if (isFileStripped || filepath !== destination) I guess when I looked at this if clause I read it as:

if the file was stripped or it does not exist at the destination then write the file

I can see how this relies on the flag to write the files that were changed only, while the unchanged ones only get written if they do not exist at the destination. I am assuming this last part, filepath !== destination, pertains to the a case when one is letting the plugin write files from src to dest.

Is that correct?


I just re-read your post again :) I think you answered my question. The use case I describe above is where I was thinking file changes in src would not make it to build if the file existed in the build folder and there were no replacements made.

As far as the request goes, I think if we separate the requests it will be easier to concentrate on separate optimizations because I do agree that the majority of file writes are non-replaced ones.

It is up to you, I would like to merge in the verbose change and push the next version.

I am grateful for your time on this.

@johny-gog
Copy link
Contributor Author

johny-gog commented Mar 23, 2019

@nuzzio

while the unchanged ones only get written if they do not exist at the destination

This is not entirely true. If they exist at the destination - they will still get written if the destination is different than source of the file.

I am assuming this last part, filepath !== destination, pertains to the a case when one is letting the plugin write files from src to dest.

Yes, if someone write files from src to dest, or using a replace method of grunt for some/all files - then it always writes the file.

It won't write the file only if destination path is the same as source path, and there was no changes.

@nuzzio
Copy link
Owner

nuzzio commented Mar 23, 2019

@johny-gog Yes, I understand. The logic here is if the source is the same as the destination then why re-write it if nothing was changed by the Grunt task. On the other hand if the source is different from the destination, then always re-write. This takes care of any edge cases with file sync. Apologies for not going over with a more careful eye. I think that it works fine, I will just add some documentation about it.

I will merge in this request, so you can leave it as is. Thanks :)

@nuzzio nuzzio changed the base branch from master to develop March 23, 2019 23:33
@nuzzio nuzzio merged commit 80991ed into nuzzio:develop Mar 23, 2019
@johny-gog johny-gog deleted the better_logs_less_writing branch April 12, 2019 12:26
@johny-gog
Copy link
Contributor Author

@nuzzio thank you! :) will update when bumped on npm. :)

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.

None yet

2 participants