Refacts building headers

[jplot]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[tobischo]

Hi @jplot,
thank you for your contribution.

I see the value in this refactoring and the idea behind.
I am not sure if the implementation is reaching the state that would be good to have as a result of this.

My main issue is having to call the init_header function before a specific order type becomes usable.
I think we should rather have the specific values either set by overwriting methods that are used by the header builder or by setting them in the initializer.

[jplot]

Hi @jplot, thank you for your contribution.

I see the value in this refactoring and the idea behind. I am not sure if the implementation is reaching the state that would be good to have as a result of this.

My main issue is having to call the init_header function before a specific order type becomes usable. I think we should rather have the specific values either set by overwriting methods that are used by the header builder or by setting them in the initializer.

Would you rather see something in this style?

class Epics::GenericRequest
  def header
    header_builder = Epics::HeaderBuilder.new(client)
    header_builder.nonce = nonce
    header_builder.timestamp = timestamp
    yield header_builder if block_given?
    header_builder.build
  end
end

class Epics::AZV < Epics::GenericUploadRequest
  def header
    super do |header_builder|
      header_builder.order_type = 'CD1'
      header_builder.order_attribute = 'OZHNN'
      header_builder.order_params = ''
      header_builder.num_segment = 1
    end
  end
end

[tobischo]

It does get rid of the init_header method

What's the point of initiating the header builder with the client just to assign every other field individually to the object afterwards?
It cannot be re-used for the next request, so the main benefit of that approach would be that the argument list for the header builder is shorter, at the expense of allowing partially constructed objects to exist.

[jplot]

It does get rid of the init_header method

What's the point of initiating the header builder with the client just to assign every other field individually to the object afterwards? It cannot be re-used for the next request, so the main benefit of that approach would be that the argument list for the header builder is shorter, at the expense of allowing partially constructed objects to exist.

The idea is to harmonize and centralize header creation and then add ebics 2.4 support.
Since ebics 2.4 requires the generation of an order identifier

In France, most banks are still on 2.4

[tobischo]

Oh, I did not question the motivation, just the technical design.

I am mostly happy with the current approach.

I see the value of the named parameters. It is a breaking change, however it should not be an issue as I do not expect anyone to directly call any of the request classes. I think I need to sleep over that.

I assume this will help with the additional changes you have in mind for supporting EBICS 2.4?

[tobischo]

Looks good. I think we should try to get consistent in the different order types

[jplot]

A solution for simplifying the construction of order parameters
jplot@a396543

[tobischo]

Looks good to me. Please consider applying your changes from jplot@a396543. I think that would make it less surprising to use the HeaderRequest class

[tobischo]

Double checked the generated headers to the previous version and they look identical