Skip to content
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

Empty tables instead of nil for optional tables #45

Open
hahawoo opened this issue Aug 2, 2017 · 14 comments
Open

Empty tables instead of nil for optional tables #45

hahawoo opened this issue Aug 2, 2017 · 14 comments

Comments

@hahawoo
Copy link
Contributor

hahawoo commented Aug 2, 2017

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
Copy link
Contributor

davisdude commented Aug 2, 2017

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
Copy link
Contributor Author

hahawoo commented Aug 2, 2017

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
Copy link
Contributor

davisdude commented Aug 2, 2017 via email

@hahawoo
Copy link
Contributor Author

hahawoo commented Aug 3, 2017

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
Copy link
Collaborator

rm-code commented Aug 3, 2017

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
Copy link
Contributor Author

hahawoo commented Aug 3, 2017

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
Copy link
Contributor

@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
Copy link
Contributor Author

hahawoo commented Aug 4, 2017

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
Copy link
Collaborator

rm-code commented Aug 4, 2017

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
Copy link
Collaborator

rm-code commented Aug 4, 2017

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

@hahawoo
Copy link
Contributor Author

hahawoo commented Aug 4, 2017

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
Copy link
Collaborator

+1 to this, it should happen with 0.11.0!

@rm-code
Copy link
Collaborator

rm-code commented Dec 4, 2017

@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
Copy link
Contributor Author

hahawoo commented Dec 31, 2017

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:

  • The developers of the projects which use love-api don't have to spend time updating and testing their code for what I must admit is quite a trivial change.
  • Having the post-processing along with everything else changes this from having no runtime logic to having some logic, and while I'm struggling to think of any practical detriments to this, I suppose it means that a line then needs to be drawn between what processing is and isn't done. With the post-processing layer I was experimenting with, I discovered that there was quite a bit of scope to what could be added besides empty tables, and I think it makes it quite a bit easier to use for some use cases, but also more complicated to learn, so it's nice that it's optional.

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

No branches or pull requests

4 participants