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

Send files via pathanmes #19

Draft
wants to merge 3 commits into
base: clos-everywhere
Choose a base branch
from

Conversation

rtmpdavid
Copy link

Dexador can automatically create multi-part post requests when given pathnames in content.

src/network.lisp Outdated
when value
if pathname-p
collect (cons (kebab:to-snake-case key) value)
else
Copy link
Member

Choose a reason for hiding this comment

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

I think here you are breaking the previous logic or this else belong to if but having a wrong indentation. Seems there is a problem with loop indentation in emacs, because for me in this small example it also indents else on the same level as when:

CL-USER> (loop for i in (list NIL NIL 1 2 3 4 5)
               when i
                 if (< i 3)
                   collect (list :before-else i)
               else
                 collect (list :else i))
((:BEFORE-ELSE 1) (:BEFORE-ELSE 2) (:ELSE 3) (:ELSE 4) (:ELSE 5))

Let's rewrite this code to make it more readable?

Also, I don't like an explicit pathname-p argument. We already have a pathname in the options. So we can decide if we wanna to make a multipart request right in the make-request itself. I'd add a helper function (multipart-request-required-p options) which will return true if at least one of options is pathname. Also pathname-p argument to process-options should be renamed to return-alist-p - this way it will be more obvious that this function can return plist or alist depending on this mode.

@svetlyak40wt
Copy link
Member

@rtmpdavid will you update this PR or I can do this myself?

@rtmpdavid
Copy link
Author

@svetlyak40wt I'll have some free time to do it this weekend.

@rtmpdavid
Copy link
Author

rtmpdavid commented Feb 24, 2024

@svetlyak40wt I've updated the pr.
Having a helper function for what amounted to a single find-if call seemed like an overkill, so I've added multipart-required to the lowermost let*.

Copy link
Member

@svetlyak40wt svetlyak40wt left a comment

Choose a reason for hiding this comment

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

Cool, I'd change find-if to some. But this you can decide youself.

collect (kebab:to-snake-case key)
and
collect value))
(multipart-required (find-if #'pathnamep options))
Copy link
Member

Choose a reason for hiding this comment

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

Probably it will be better to use `(some #'pathnamep options) here, because we don't need a pathname as the result:

CL-USER> (some #'pathnamep (list 1 2 #P"foo" #P"bar" 5 6))
T
CL-USER> (find-if #'pathnamep (list 1 2 #P"foo" #P"bar" 5 6))
#P"foo"

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.

2 participants