-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Generation: Add generation for external files #465
Conversation
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.
👏
module.exports = function downloadEspresso(fullyQualifiedClass) { | ||
const tmpDir = os.tmpdir(); | ||
const path = fullyQualifiedClass.replace(/\./g, "/"); | ||
const fileContent = downloadFileSync( |
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 suggest you use concurrency here, this may take long time downloading files individually. I know there are a few download managers on npm that do exactly that.
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.
Isn't that a premature optimization? I mean this code is only executed while generating code 🤷♂️ Do you have a good opportunity at hand?
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.
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 gave it a try, but it made the code unnecessarily complicated to read, while I don't see this big speedup opportunities (we are talking about maybe 10 file in total, I guess). I would like to leave it this simple, but feel free to add the download package in 👍
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.
cool, had no idea were talking about 10 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.
recounted, it's two 😆
@@ -18,6 +18,7 @@ | |||
"devDependencies": { | |||
"babel-generator": "^6.25.0", | |||
"babel-types": "^6.25.0", | |||
"download-file-sync": "^1.0.4", |
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.
You're not using 'download' dependency anywhere, am I missing something?
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.
fixed that 👍
30749f7
to
5906084
Compare
5906084
to
e518fe0
Compare
e518fe0
to
3fb0c54
Compare
3fb0c54
to
5e7e76e
Compare
5e7e76e
to
708c85a
Compare
@rotemmiz Tests seem green ✅ 66 passing (5m)
4 pending |
This PR allows us to get the source code of espressos APIs (which are not checked in to this project) and generate code from them. I used it for three actions, other ones will be quick wins (by adding the String type)