Skip to content

Add fortran-curl as dependency and a minimal GET request example #5

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

Merged
merged 3 commits into from
May 26, 2023

Conversation

rajkumardongre
Copy link
Contributor

Add fortran-curl as dependency and a minimal GET request example, also added a simple_get.f90 file in example folder

Copy link
Member

@interkosmos interkosmos left a comment

Choose a reason for hiding this comment

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

Just a few remarks:

  • The method attribute in request_type and response_type should probably be of type integer.
  • The status_code in response_type has no default value. HTTP status codes should be added as public parameters. The attribute content_length should be an 8-byte integer, as it is available in Fortran on 32-bit platforms too.
  • Some procedure dummy arguments still need proper intent attributes.
  • In new_request(), switch case is preferable to if … then … else blocks.
  • The function client_get_response should not stop on error but return an error code/message instead.

@rajkumardongre
Copy link
Contributor Author

Alright, I'll make the required changes

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @rajkumardongre. I suggest also organizing defining the derived types and their related procedures in their respective modules, and use http_client module only to make available the public API, i.e. response_type, http_request, HTTP_GET etc.

Comment on lines 53 to 57
character(len=*) :: url
integer, optional :: method
type(request_type) :: request
type(response_type) :: response
type(client_type) :: client
Copy link
Member

Choose a reason for hiding this comment

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

Just one space around ::, no need for all this whitespace.

Comment on lines 97 to 103
rc = curl_easy_setopt(curl_ptr, CURLOPT_URL, this%request%url // c_null_char)
! setting request method
rc = curl_easy_setopt(curl_ptr, CURLOPT_CUSTOMREQUEST, this%request%method // c_null_char)
! setting callback for writing received data
rc = curl_easy_setopt(curl_ptr, CURLOPT_WRITEFUNCTION, c_funloc(client_response_callback))
! setting response pointer to write callback
rc = curl_easy_setopt(curl_ptr, CURLOPT_WRITEDATA, c_loc(response))
Copy link
Member

Choose a reason for hiding this comment

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

Here as well, and elsewhere, just one space after commas.

@rajkumardongre
Copy link
Contributor Author

I suggest also organizing defining the derived types and their related procedures in their respective modules, and use http_client module only to make available the public API, i.e. response_type, http_request, HTTP_GET etc.

Thanks, @milancurcic. So basically, I need to generate four files.

  1. request.f90
  2. response.f90
  3. client.f90
  4. `http-client.f90

request.f90 should contain the definition of the request_type along with its corresponding helper procedures. response.f90 should have the definition of the response_type and its helper procedures. Similarly, client.f90 should contain the client_type and its helper procedure. Finally, http-client.f90 will provide access to the public API.

@arteevraina
Copy link
Member

Thanks, @milancurcic. So basically, I need to generate four files.

  1. request.f90
  2. response.f90
  3. client.f90
  4. `http-client.f90

request.f90 should contain the definition of the request_type along with its corresponding helper procedures. response.f90 should have the definition of the response_type and its helper procedures. Similarly, client.f90 should contain the client_type and its helper procedure. Finally, http-client.f90 will provide access to the public API.

Yeah, Looks good. But maybe use snake_case for naming http_client.f90 as well ?

@milancurcic
Copy link
Member

@rajkumardongre check the recent fpm module and source naming requirements: https://fpm.fortran-lang.org/en/spec/naming.html

Because the default __ thing is kinda ugly, we may want to use a custom prefix (https://fpm.fortran-lang.org/en/spec/naming.html#custom-module-names) so perhaps our structure would be:

  • http top-level module (public API)
  • http_request
  • http_response
  • http_client

(and having the http as the prefix also allows the possibility of adding http_server in the future)

Another thing we should decide early is whether to use the old and inappropriate .f90 for source suffixes, or to just go with .f (now that free form is assumed, https://fpm.fortran-lang.org/en/spec/manifest.html#source-form). Since we just started, this project may be a perfect guinea pig to find out if there are any unforeseen issues with using .f for free-form sources.

So, my suggestion for this PR is to have these source files: http.f, http_client.f, http_request.f, and http_response.f.

Let me know what you all think.

@rajkumardongre
Copy link
Contributor Author

Because the default __ thing is kinda ugly, we may want to use a custom prefix (https://fpm.fortran-lang.org/en/spec/naming.html#custom-module-names) so perhaps our structure would be:

  • http top-level module (public API)
  • http_request
  • http_response
  • http_client

Thank you, @milancurcic. I believe using a custom prefix would be a good approach, but I do have a concern. Our package name is http-client, which means that fpm will read it as http_client. According to fpm, the top-level module name must be the same as the package name. However, we are already using http_client as a module, which is not a top-level module. This could potentially confuse other developers about what our top-level module is: http or http_client To address this concern, can we rename our http_client module to something else, such as http_curl, http_server, or something else, while keeping http as our top-level module.

@milancurcic
Copy link
Member

I see, good point. In that case, should we rename our package to just http? Shorter, simpler (no hyphen caveats), although less specific, but that's probably fine.

@rajkumardongre
Copy link
Contributor Author

rajkumardongre commented May 25, 2023

Yes, I also think http will be a good fit for our package name.

So now we can follow this structure :

  • http top-level module (public API)
  • http_request
  • http_response
  • http_client

@milancurcic
Copy link
Member

OK, let's go ahead then and rename it in pyproject.toml and I'll rename the repo after we merge the PR.

@milancurcic
Copy link
Member

It just occurred to me that this PR is also on a branch on fortran-lang/http-client rather than your fork (rajkumar/http-client). Please work only on development branches on your fork going forward. I'll merge this PR as is to not complicate further, and let's tackle the outstanding comments in separate PRs.

@milancurcic milancurcic merged commit 45600b0 into main May 26, 2023
@milancurcic milancurcic deleted the rajkumar branch May 26, 2023 21:47
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.

4 participants