Skip to content

Conversation

@iaddict
Copy link
Contributor

@iaddict iaddict commented Jan 10, 2014

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c85b0af on iaddict:new-api-cmxums into 1d91d64 on romanlehnert:master.

Copy link
Owner

Choose a reason for hiding this comment

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

Seems a bit odd to me to prefix the hash keys with numbers. At least this is not the current practice in this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well - yes when looking at it it looks odd - but when I implemented the api and I looked at the API documentation at collmex it was super hard to find the matching fields (translate to german and find the right position).

The API is CSV based and thus the index is the most important criteria to feed it right.
When using the ruby API it gets so much easier to find and use the right fields. It just feels better to have the index in place and to be safe when using the API. The descriptive name makes it then just easier to read.

@romanblick
Copy link
Owner

Could you please rebase against the master branch?

  which is ugly as hell, but makes following the api documentation so much easier and usage less error prone
@iaddict
Copy link
Contributor Author

iaddict commented Jan 28, 2014

Rebased :)

@romanblick
Copy link
Owner

Could you imagine a syntax that taks the api fild number and a param as value, when building such a command? In that case, we could prevent prefixing the hash keys and use both options.

I mean, the specification of the commands follows the order of the collmex api. And actually it is possibl to give the Line object an array as param:

Collmex::Api::Login.new([12,34])

I'm thinking about sth like

Collmex::Api::Login.new do |fields|
   fields[key] = "bla"
end

Where key can be a hash key or an array index - what would enable you to do:

Collmex::Api::Login.new do |fields|
  fields[3] = "hallo"
  fields[4] = "iaddict"
end

@iaddict
Copy link
Contributor Author

iaddict commented Feb 5, 2014

Well, that would be kind of better. Albeit I think that a lot of (documentation) information is lost when only using the index. The "beauty" of the "ugly" hash keys that contain both, the index and the description, is that on the one side you can see what the key means as well as which index it is and on the other side that code completion works.

What do you think about a change in the api definition along the lines of distinction of the usage of Symbol vs. String keys. The api will be defined by strings as keys with a less ugly syntax (e.g. '2 Customer Id') which gets transformed to an alternative key as Symbol which misses the number (e.g. :customer_id).

Or possibly the other way around: lets define the keys as symbols (as it is now). We already have an index. So we may map a String with a number as prefix to the symbol.

Then it would be possible to use it in these flavors:

Collmex::Api::Login.new do |fields|
  fields["1 Satzart"] = "hallo" # this is just pasted directly from the collmex api description :)
  fields["4 customer_id"] = "iaddict"
  fields[:the_code_api_key_as_symbol] = "..."
  fields[4] = "..." # with an index
end

The only things that are missing - in my view - is an easy way to author API lines and code completion when using the api. The code completion issue is mitigated by the possibility to just copy the original documentation.

I started of writing the "ugly" api line definition because I got lost in the sheer amount of lines which my eyes couldn't follow and keep the relation to the index based api description.

By the way is there an english api description? Because I even thought about a generator that just takes the original description and transforms it to the ruby specification. Also the keys then would be in german (but then one would just have to translate them).

Ok. Lots of thoughts. What do you think. :)

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.

3 participants