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

Add attachments #237

Merged
merged 1 commit into from
Jul 19, 2019
Merged

Add attachments #237

merged 1 commit into from
Jul 19, 2019

Conversation

allanmayanei1
Copy link

Description

Add support attachments the file

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@slnode
Copy link

slnode commented Jul 16, 2019

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

src/http.js Outdated

if (typeof data === 'string') {
if (typeof data === 'string' && attachments.lemgth === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, it should be attachments.length.

src/http.js Outdated
var mergeOptions = ['headers'];
if (exoptions) {
var attachments = exoptions.attachments || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove var.

src/http.js Outdated

if (typeof data === 'string') {
if (typeof data === 'string' && attachments.lemgth) {
Copy link
Contributor

Choose a reason for hiding this comment

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

attachments.lemgth => attachments.length === 0?

Copy link
Author

Choose a reason for hiding this comment

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

I did not understand what the correction would be

Copy link
Contributor

Choose a reason for hiding this comment

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

Please note length is correct instead of lemgth.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you are testing if the attachments array is empty.

Copy link
Author

Choose a reason for hiding this comment

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

silly mistake, now it goes

@raymondfeng
Copy link
Contributor

@allanmayanei1 It would be good to add a test case for this new feature.

@raymondfeng
Copy link
Contributor

@allanmayanei1 The changes look good now. Would you please squash all commits into one?

@allanmayanei1
Copy link
Author

@allanmayanei1 The changes look good now. Would you please squash all commits into one?

changes will rise when ?

@raymondfeng
Copy link
Contributor

@allanmayanei1 I have cleaned up the commit history in your repo.

@allanmayanei1
Copy link
Author

@raymondfeng Thank you, something is missing?

@raymondfeng raymondfeng merged commit 666d31f into loopbackio:master Jul 19, 2019
@raymondfeng
Copy link
Contributor

Merged and published. Thanks for the contribution.

@allanmayanei1
Copy link
Author

I thank you

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.

None yet

3 participants