-
Notifications
You must be signed in to change notification settings - Fork 5
Extract the io_file
POSIX API into a separate package
#272
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
Conversation
io_file
POSIX API into a separate package
@brianquinlan - for the review - is this mostly a mechanical change? Create a new labs package, move content into it, and reference it from the existing package? |
Yes. I added a very simple test (which I will improve in follow-up PRs) and a README file. Everything else is a move from the existing location or was created from BTW, I don't actually want a good name for this package - hopefully someone will fork this and take whatever the best available name is ;-) |
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.
lgtm w/ some in-line comments
meta: ^1.16.0 | ||
native_toolchain_c: ^0.16.0 | ||
path: ^1.9.1 | ||
unix_api: |
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'd try using something like ^0.1.0
here, and add a dependency_overrides section further down:
dependency_overrides:
unix_api:
path: ../unix_api
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.
Done.
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 changed to back because changing this also trickles into the mobile_test subproject and it seemed better to contain the path reference here. WDYT?
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.
Well, it's not normally how we'd try and reference a package - it'll mean that io_file
won't be useable outside this repo. I'm not sure what the issue is w/ the subproject.
Feel free to use your best judgement here though; we're not trying to ship this currently. If you do use a relative reference into this repo, you might leave a comment that this isn't our normal practice.
meta: ^1.16.0 | ||
native_toolchain_c: ^0.16.0 | ||
path: ^1.9.1 | ||
unix_api: |
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.
Well, it's not normally how we'd try and reference a package - it'll mean that io_file
won't be useable outside this repo. I'm not sure what the issue is w/ the subproject.
Feel free to use your best judgement here though; we're not trying to ship this currently. If you do use a relative reference into this repo, you might leave a comment that this isn't our normal practice.
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.