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

Android replaceInFile #1197

Merged

Conversation

davidgovea
Copy link
Contributor

addresses #1060

Rebased the old POC up to master -- figured I'd create the PR to help discussion.

With iOS's plugin processing, the 'hull filling' happens first. In android, it seems to happen after the plugin processing, so we need to defer the replacements.

The fs use and replacement function was lifted from the iOS implementation, but was then wrapped in an async function side-effect creator for later processing. This is probably inefficient, but I held back on optimizations, pending discussion.

Copy link
Member

@belemaire belemaire left a comment

Choose a reason for hiding this comment

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

@davidgovea
I'm good with this implementation.
The processing in two parts is not ideal, but that's due to the current class design.
These generators classes are anyway overdue for refactoring, so we will clean things up a bit soon hopefully, that's why I'm not too concerned at this point.

Could you just make the async replacement functions non async. Looking at the implementation, there is no await in the anonymous function body, and it's only making fs sync calls so there is nothing async here ;) So just update the replacements typedef, remove async and await when you invoke each function in the for loop.

Then could you just please squash your commits to a single one :)

@davidgovea davidgovea force-pushed the feature/android-replaceinfile branch from dcf66ba to a1fe481 Compare June 5, 2019 22:40
@davidgovea
Copy link
Contributor Author

@belemaire updated & squashed

@belemaire
Copy link
Member

Thanks 👍

@belemaire belemaire merged commit 8782f11 into electrode-io:master Jun 5, 2019
@friederbluemle friederbluemle changed the title WIP: Android replaceInFile Android replaceInFile Dec 20, 2021
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.

2 participants