Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement StructureMap-based extraction #566
Implement StructureMap-based extraction #566
Changes from all commits
e3468fb
06fe548
4a94690
890f9aa
afd1a26
aa746bf
3e8609e
d6fc9ab
712aa4b
39201b3
214e34c
d535675
134e7ae
449cb92
30feb3f
bccfb2c
cc9f44e
e9015c8
d8b9cb3
8c1354c
87812f0
283ac5f
b4bc795
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
looking at this file i'm still slightly confused by the apis.
it still feels like it's doing too much. can we maybe have 2 classes
NpmPackageProvider
SimpleWorkerContextProvider
so in the former you have
getNpmPackage(context: Context)
and in the latter you havegetSimpleWorkerContext(context: Context)
. The latter depends on the former. And both caches what needs to be cached within themselves.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.
I have split the functionalities into separate files
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.
internal
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.
Can we leave this as public?
The whole initial setup process takes around 3 minutes on a good device and I plan to have this happening after installation and application start so that extraction is fast having done this previously.
The target application would have to setup the module after installation and load the modules at application start
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.
3 minutes! that's mad... can you add a documentation here to say this process is very time consuming
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.
I see this is is already documented here.
I'll add extra documentation on the specific methods and some guiding documentation on the class itself
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.
general comment: the file IO functions should be
suspend
functions