foundeo / cfdocs

Repository for the cfdocs.org site.
https://cfdocs.org/
Other
180 stars 342 forks source link

Syntax for functions and methods #525

Open shaedrich opened 7 years ago

shaedrich commented 7 years ago

Now we have the keys syntax and member but member is definitely syntax as well. So that's a confusing naming, I guess. Would it be better to differentiate between function_syntax and method_syntax?

pfreitag commented 7 years ago

I'm not sure if that would be more clear, maybe you can give me a more specific example of what it could look like.

shaedrich commented 7 years ago

Something like that:

{
    "name": "",
    "type": "",
    "syntax": [{
        "function": "ArrayReduce(arr, function() {})",
        "method": "arr.reduce(function() {})"
    }],
    ...
}
KamasamaK commented 7 years ago

@shaedrich I think we should be very careful with changes like that which would break compatibility with the current format.

While we are discussing potential changes for representing member functions, I have a couple other ideas to throw out there.

  1. Include member type. For the example of arrayReduce above, it should be array.
  2. Include engine information with the same format as the entity itself and the same proposed for parameters.
  3. Some distinct fields such as member_name would be nice instead of having to parse the syntax string.
shaedrich commented 7 years ago

@KamasamaK You are right. But we could run a migration on the json files. Would be necessary when reworking the whole syntax anyway.

KamasamaK commented 7 years ago

@shaedrich The migration isn't so much the issue as other tools using CFDocs. The Sublime Text plugin retrieves the JSON from the GitHub master repo, and parsing would break with a change like this. It would be good to get ahead of that and notify them of any breaking changes before they happen, but I wonder how many other projects or tools rely on such a process that we are unaware of.

shaedrich commented 7 years ago

So we could performe none of the recommended changes.

shaedrich commented 7 years ago

Maybe we should launch another branch.

Are the data fetched by the api by most of the plugins unsing cfdocs?

This could be made better when provide an api versioned by semver.

KamasamaK commented 7 years ago

The only plugin I am aware of that currently uses CFDocs is sublimetext-cfml. A versioned API would absolutely solve this issue, but I was just bringing up the issue as to the current capabilities. If it's decided to go in the direction of making breaking changes, then a new branch would be appropriate and can be merged into master when it's deemed ready.

shaedrich commented 7 years ago

So what about creating a new branch first, do the breaking changes, perhaps set up an API to prevent us from running into something like that again? If jcberquist agrees to adapt our changes, we can merge the new branch into master

shaedrich commented 7 years ago

So what about creating a new branch, doing our changes there, perhaps set up an API which prevents us from running into something like that again, then ask jcberquist if he agrees to adapt our changes and finally merge them into master?

pfreitag commented 7 years ago

I want to be pretty careful about breaking backwards compatibility for a few reasons.

1) There are already a few projects using the repository (sublimetext-cfml being one of them), I think it uses the json files directly so it is not as simple as building a versioned API over http. 2) It will take a lot of work to migrate existing docs to another format even if only a minor change, I don't think this project has the resources to pull that off right now - we don't even have all of the Lucee 5 new features documented yet.

My question is --- could we add some metadata to params objects in order to be able to auto generate all the valid syntaxes?

shaedrich commented 7 years ago

I know...

In my opinion these changes would be recommended to be done sooner or later.

Migration could be done by a script. That does not take that much effort. The script can't write texts, but it can migrate the syntax to fit the new requirements in all docs at once.

What sort of metadata?

KamasamaK commented 7 years ago

@pfreitag I believe we can add more data that can be used to auto-generate the syntaxes, but I don't believe it would be optimal to just add to params. This is just a first-draft, so suggestions for improvement are welcome. I have excluded some irrelevant properties.

{
  "name": "arrayReduce",
  "type": "function",
  "member_details": {
    "name": "reduce",
    "type": "array"
  },
  "params": [
    {
      "values": [],
      "default": "",
      "description": "the input array",
      "name": "array",
      "type": "Array",
      "required": true
    },
    {
      "values": [],
      "default": "",
      "description": "Closure or a function reference that will be called for each of the iteration. The arguments passed to the callback are\r\n\r\nresult: result of the reduce operation after the previous iteration\r\nitem: item in the array\r\nindex : current index for the iteration\r\narray : reference of the original array",
      "name": "function",
      "type": "function",
      "required": true,
      "callback_params": [ 
        {
          "values": [],
          "default": "",
          "description": "result of the reduce operation after the previous iteration",
          "name": "result",
          "type": "Any",
          "required": true
        },
        {
          "values": [],
          "default": "",
          "description": "item in the array",
          "name": "item",
          "type": "Any",
          "required": true
        },
        {
          "values": [],
          "default": "",
          "description": "current index for the iteration",
          "name": "index",
          "type": "Numeric",
          "required": false
        },
        {
          "values": [],
          "default": "",
          "description": "reference of the original array",
          "name": "array",
          "type": "Array",
          "required": false
        }
      ]
    },
    {
      "values": [],
      "default": "",
      "description": "Initial value which will be used for the reduce operation. The type is any.",
      "name": "initialValue",
      "type": "Any",
      "required": false
    }
  ]
}

Some notes about the changes:

pfreitag commented 7 years ago

@KamasamaK thanks for that, that looks good to me.

So here is one possible way of documenting multiple syntaxes in the params would be to add two new keys:

required_when : array of conditions for example isValid min argument applies only when type argument is range...

required_when: [ { "param": "type", "condition":"equals", "value":"range" } ]

Then the doc for that argument could say required when type="range"

position : integer denoting the argument position, it will default to the position of the argument in the params array, but if specified allows you to document two different arguments occurring at the same position. So again for the isValid example you might have something like this:

"params": [
    { "name":"type", position:1 },
    { "name":"value", position:2 },
    { "name:"min", position:3 },
    { "name":"max", position:4 },
    { "name":"pattern", position: 3}
]

From that you might be able to build all the syntaxes programatically. Now I'm not sure if there are cases that are more complex where this wouldn't work well, it is probably likely.

KamasamaK commented 7 years ago

I would also like to note that there are functions such as setDay that are only member functions, so there would need to be a way to indicate that as well.

shaedrich commented 7 years ago

@KamasamaK Could be reflected by the breadcrumbs: CFDocs > All Functions and Tags > Functions > Member Functions

shaedrich commented 6 years ago

Maybe we should ask jcberquist what he's thinking about these suggestions and whether he would be willing to implement them (step by step, with a huge update or with two APIs: one current and one legacy stable).

KamasamaK commented 6 years ago

Regarding my suggestion for member_details, I've found a further consideration. The member function can have a different return type. arrayAppend and array.append() is an example of such a discrepancy, which return a boolean and an array respectively as of CF2016. Incidentally, they were both boolean in CF11, so there is an additional consideration of return types varying based on engine.

shaedrich commented 6 years ago

Sounds good :+1:

pfreitag commented 6 years ago

Yeah I'm good with adding the member_details - I think it makes sense and could be quite useful for a number of reasons.

KamasamaK commented 6 years ago

I am looking at updating the schema for member_details. I'm largely using the ColdFusion member functions document and Lucee Objects article as a basis. Looking for feedback on the following:

  1. I think we should have type go beyond the standard CFML types we normally use. The types it would be useful to additionally include are image, locale (Lucee-only), spreadsheet (CF-only), and timezone (Lucee-only).

  2. I am thinking of including a returns property and making it required.

  3. Yet again we have the issue of pointing out engine differences. Right now, the top-level engines object is insufficient for providing certain information that's easily parsable -- of course we can write anything into the notes. I can add an engines object into member_details, but it would only be useful to indicate support for the member function itself and not for things like a return type that differs between engines or even between versions of an engine an in my example above.

  4. Above, I said "This assumes that member syntax is always the same as standard, but with the first param moved to be the object". Does anyone know of any case where this is not true? If it is true, then we would not have to bother with an additional params solution. It might even make type unnecessary if not for the helpfulness of certain non-standard types as outlined in 1.

KamasamaK commented 5 years ago

I have an initial commit at 2d0a1c4, but I would still be interested in some feedback before I make a PR.

  1. I did not include those in the initial commit, but I still think it would be weird for type to be any since the member functions would only apply to those specific types.
  2. This does not seem controversial so I included it.
  3. I included another engines object here as I did with params so that engine support can more specifically be conveyed.
  4. I'm still not aware of a situation where this is not true, though I would prefer to have a more robust solution than to spec around this always being the case.
shaedrich commented 5 years ago
  1. Absolutely
  2. no complains
  3. :+1:
  4. no sure about that 🤔

overall: LGTM