Core support for embedded files. Name dictionary entry #47
Conversation
|
Looks good to me. |
|
Great! Anything else needed to be approved? |
|
Maybe some specs, @pointlessone will know and decide when there is time. |
There was a problem hiding this comment.
Overall I'm hesitant to approve this.
This only adds an API for embedded files names dictionary. But doesn't even do that as this module is not included anywhere. It doesn't handle actual file descriptors creation. The API itself doesn't add much as the specific data structures are exposed anyway. It doesn't enable any new use case just makes already available one slightly less verbose.
I'd suggest starting with a prawn PR that demonstrates the feature. Describe the use case, show how new API makes it possible or much more ergonomic. I suspect there already might be a counterpart just not published because it depends on this PR. But without that PR this doesn't make much sense.
Lack of tests is slightly worrying, too.
|
I added some tests for the module using a mock class. Also, I created the PR with the feature in prawn 1214 |
|
Hi @pointlessone, I'm working on the tests of the prawn PR 1214 but, in order to work properly and pass the checks, it requires this one. Do you think it's ok? |
|
Yeah. Add in that PR description that it depends on this one. Make sure that both are green and I'll run them locally to check. Then we'll rerun build on prawn PR once this is merged. |
|
Hey @pointlessone, I have mentioned this PR on the one in Prawn, that one is green (without actually using these changes, so it's "fake" green until then) but I'm not sure why the checks of this repo are stalled. Do I need to do something else in particular? |
|
I'm not sure… Maybe rebase and push it again? |
dvdalilue
force-pushed
the
feature/embedded-file-support
branch
from
9823013 to
a1aa484
Compare
|
It seems that worked. Now is green, thanks |
|
Hi @pointlessone, just wondering if you were able to check this PR? |
For a project, I need to implement the file attachment feature. So given that I'm using prawn already, I just extended the modules and classes to support that.
This PR defines a core module for embedded files that will be used by the prawn structures, something that is defined in my project. The idea is to have full support to attach files in prawn. So I have the intention to create a PR in prawn as well but it depends on this one.
Looking forward to any comments.
Thanks in advance.