search

love2d-community / love-api

The whole LÖVE wiki in a Lua table.
http://love2d-community.github.io/love-api/
300 stars 48 forks source link

Empty tables instead of nil for optional tables #45

Open hahawoo opened 7 years ago

hahawoo commented 7 years ago

Currently, one has to check if an optional table (i.e. functions, types, enums, returns, arguments, constructors, supertypes, subtypes) exists before looping though it, like this:

if variant.arguments then
    for _, argument in ipairs(variant.arguments) do
    end
end

I think it would be nicer to use if had an empty table instead of nil, so things could be looped through without checking if they exist first.

To avoid adding unnecessary stuff to the files, I suggest that the empty tables be added after the table is created, i.e.:

local api =  {
    -- etc.
}

local function functions(f)
    for _, function_ in ipairs(f) do
        for _, variant in ipairs(function_.variants) do
            if not variant.returns then variant.returns = {} end
            if not variant.arguments then variant.arguments = {} end
        end
    end
end

local function types(t)
    for _, type_ in ipairs(t) do
        if not type_.functions then type_.functions = {} end
        if not type_.constructors then type_.constructors = {} end
        if not type_.supertypes then type_.supertypes = {} end
        if not type_.subtypes then type_.subtypes = {} end

        functions(type_.functions)
    end
end

functions(api.functions)
functions(api.callbacks)
types(api.types)

for _, module_ in ipairs(api.modules) do
    if not module_.functions then module_.functions = {} end
    if not module_.types then module_.types = {} end
    if not module_.enums then module_.enums = {} end

    functions(module_.functions)
    types(module_.types)
end

return api

This change can break things if the script using it is checking to see if something exists for purposes other than looping, so sometimes if variant.arguments then has to become if #variant.arguments > 0 then.

I don't want to make this change now and unexpectedly break people's scripts, but I think it's a worthwhile change, so maybe scripts using the current love-api table could use the above code to test and see if it breaks their code (and enjoy simplifying their code because of the change!), and if it all works then love-api could be changed.

I've updated html-generator.lua to use this empty table format, I'll make a pull request for it.

davisdude commented 7 years ago

Personally, I would prefer the table to not be present if it has no content. In my opinion, it's easier to do 

for i, v in pairs( tab or {} )

Instead of doing 

if #tab ~= 0 then 
-- loop
hahawoo commented 7 years ago

I hadn't thought of for i, v in pairs( tab or {} ), that's neat! But with empty tables it's even easier, because you wouldn't have to do that or:

if #tab ~= 0 then 
-- loop

You can just loop, because if there are no items in the table the loop block won't run but it also won't error, it will just skip over it. Currently you do have to check before looping, or do the neat thing you mentioned.

davisdude commented 7 years ago

Having the table present but empty is kind of problematic for me, though. See my project here. Basically, if, for instance, the types table is empty, it shouldn't have a "types" sub heading. (I guess it could have one and just say "none," but you see my point)

I understand if you want to change it anyway, it's just something to consider. Either way has pros and cons.

hahawoo commented 7 years ago

I don't think you'll need to change a thing actually. Unless I've made a mistake, your code will produce exactly the same output with empty tables as it currently does. You can verify this yourself by using the above code after you require love-api, i.e.:

local api = require 'love-api.love_api'

local function functions(f)
    for _, function_ in ipairs(f) do
        for _, variant in ipairs(function_.variants) do
            if not variant.returns then variant.returns = {} end
            if not variant.arguments then variant.arguments = {} end
        end
    end
end

local function types(t)
    for _, type_ in ipairs(t) do
        if not type_.functions then type_.functions = {} end
        if not type_.constructors then type_.constructors = {} end
        if not type_.supertypes then type_.supertypes = {} end
        if not type_.subtypes then type_.subtypes = {} end

        functions(type_.functions)
    end
end

functions(api.functions)
functions(api.callbacks)
types(api.types)

for _, module_ in ipairs(api.modules) do
    if not module_.functions then module_.functions = {} end
    if not module_.types then module_.types = {} end
    if not module_.enums then module_.enums = {} end

    functions(module_.functions)
    types(module_.types)
end

The reason why this is the case is, taking "types" as an example:

extractSubData( module, 'types', '', ':' )

This calls extractSubData, which then gets the types table, and then checks if the number of items in the types table is greater than 0. If it's 0, then the section is not added and the function returns:

local function extractSubData( module, sectionName, prefix, funcSeparator )
    local section = module[sectionName]
    if section and #section > 0 then

You can optionally simplify your code if you wanted to though, because instead of lines like this:

if module.supertypes and #module.supertypes > 0 then

you could have lines like this, without the need for checking if the table exists:

if #module.supertypes > 0 then
rm-code commented 7 years ago

I don't mind the additional checks for existing tables personally, but I can see how this would benefit other people of course.

If we decide to go this way, we need to make sure the documentation is updated accordingly.

I currently don't have a development machine so I can't test your changes in my two projects depending on love-api (love-atom and love-IDEA), but I'll try to do so ASAP.

hahawoo commented 7 years ago

No rush. :)

I looked through the projects that use it and found the following changes which would need to be made:

LÖVE API for Notepad++:

Line 90:

if #variant.arguments > 0 then

Line 98:

if #variant.returns > 0 then

Line 107:

if #variant.arguments > 0 then

love-atom:

Line 73:

if #vdef.arguments > 0 then

Line 89:

if #f.variants[1].returns > 0 then

Line 188:

if #type.supertypes > 0 then

Optionally, the if statements on lines 164 and 182 can be removed.

ZeroBrane Studio

On a new line after v.supertypes = nil:

v.subtypes = nil

Before:

if v.variants and #v.variants > 0 then
  v.returns = params(v.variants[1] and v.variants[1].returns or v.variants[2] and v.variants[2].returns)
end
if v.variants and #v.variants > 0 then
  v.args = params(v.variants[1] and v.variants[1].arguments or v.variants[2] and v.variants[2].arguments)
end

After:

if v.variants and #v.variants > 0 then
  v.returns = params(v.variants[1] and #v.variants[1].returns > 0 and v.variants[1].returns or v.variants[2] and #v.variants[2].returns > 0 and v.variants[2].returns)
end
if v.variants and #v.variants > 0 then
  v.args = params(v.variants[1] and #v.variants[1].arguments > 0 and v.variants[1].arguments or v.variants[2] and #v.variants[2].arguments > 0 and v.variants[2].arguments)
end

Before:

or v.types and "class"
or v.constants and "class"
or v.functions and "lib"

After:

or v.types and #v.types > 0 and "class"
or v.constants and #v.constants > 0 and "class"
or v.functions and #v.functions > 0 and "lib"

LÖVE Hints for Brackets.io

I don't understand this code deeply so maybe there is a nicer way of doing this (or maybe nothing needs to be done), but one solution is to reverse the changes to the love-api table and make empty tables nil:

local function functions(f)
    for _, function_ in ipairs(f) do
        for _, variant in ipairs(function_.variants) do
            if #variant.returns == 0 then variant.returns = nil end
            if #variant.arguments == 0 then variant.arguments = nil end
        end
    end
end

local function types(t)
    for _, type_ in ipairs(t) do
        if #type_.functions == 0 then type_.functions = nil end
        if #type_.constructors == 0 then type_.constructors = nil end
        if #type_.supertypes == 0 then type_.supertypes = nil end
        if #type_.subtypes == 0 then type_.subtypes = nil end

    if type_.functions then
            functions(type_.functions)
    end
    end
end

functions(api.functions)
functions(api.callbacks)
types(api.types)

for _, module_ in ipairs(api.modules) do
    if #module_.functions == 0 then module_.functions = nil end
    if #module_.types == 0 then module_.types = nil end
    if #module_.enums == 0 then module_.enums = nil end

    if module_.functions then
        functions(module_.functions)
    end

    if module_.types then
        types(module_.types)
    end
end

LÖVE-IDEA and Vim LOVE Docs seem to be fine without changes, and the changes needed for the quick reference are in a pull request.

davisdude commented 7 years ago

@hahawoo You're right, I don't need to change anything because the generation function is recursive, so even objects which won't have a types attribute, such as a function, are still checked, so it is necessary to check even if they are blank. I realize that's a corner case, I was mostly just mentioning it as an example.

Additionally, some (though very few) modules already contain empty tables (for instance, here).

Either way, I'm not against the change. In fact, if you look at most web-apis, it's probably more standard to return an empty field instead of not including it at all, at least in my experience.

hahawoo commented 7 years ago

Good point about the modules already containing empty tables, I didn't realise that, I guess that wouldn't be necessary with the post-processing I'm suggesting.

It's also how LOVE does it as well in a sense, I think love.joystick.getJoysticks for example returns an empty table if there are no joysticks.

rm-code commented 7 years ago

Yeah it's probably the best way to go. I'm all for it :)

Good point about the modules already containing empty tables, I didn't realise that, I guess that wouldn't be necessary with the post-processing I'm suggesting.

Cool, it would help reducing the overall size and "noise"!

rm-code commented 7 years ago

@hahawoo We could open PRs on said repos, or at least report the necessary changes. I probably can help next week.

hahawoo commented 7 years ago

Maybe we could coordinate it with the next LOVE release, so when love-api is updated for the new version users of it can update their code at the same time that they're generating new files, to save people from testing the new code and then having to use it again when the new version is released. Either way I guess we could still make PRs at any time with a note saying "don't pull this juuust yet".

pablomayobre commented 6 years ago

+1 to this, it should happen with 0.11.0!

rm-code commented 6 years ago

@hahawoo I started a 0.11.0 branch. Feel free to add the empty tables on top of that. It'll be merged once 0.11.0 is released.

hahawoo commented 6 years ago

That's really cool that it'll be updated for 0.11.0!

I'm hesitant to make this change now, because I think I prefer the "post-processing layer" I experimented with in love-api-experiments (by the way, feel free anytime to take anything from that project into this project if it seems like a good idea).

The main reasons are: