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

Simplify.... #38

Open
crisward opened this issue Feb 27, 2017 · 7 comments
Open

Simplify.... #38

crisward opened this issue Feb 27, 2017 · 7 comments
Labels

Comments

@crisward
Copy link
Contributor

crisward commented Feb 27, 2017

After reading over the code, I can't help thing kemal-session could be significantly simplified by just saving a single object. That is to say, removing all the strings, int32, int64, floats, bools etc.

You could tell users they had to define a storable object. They could then define the structure of the data they want to save. I typically want to save a user, so as long as my user has a to_json method (which is probable if I run an endpoint on it) I can the just nest in as a sub object.

ie

class SessionSchema
    def initialize
    end

    JSON.mapping({
      user:   {type: User, nilable: true},
      basket: {type: Basket, nilable: true},
      flash:{type:String, nilable: true},
    })
    include Session::StorableObject
  end

This would remove many of the current macros and possibly make it easier for developers to add other store engines.

What do you think?

@sdogruyol
Copy link
Member

Not sure about this. Let's ask @neovintage and @Thyra

@crisward
Copy link
Contributor Author

@neovintage flash plugin would need modifying a little. He'd need to ask the author to add a flash key to their session schema for his flash object. But that wouldn't be that bad.

@Thyra
Copy link
Contributor

Thyra commented Mar 3, 2017

Phew, that's a difficult question. I don't think it would really make such a big difference for us: There would be less macros, but the methods for managing primitive types are still needed; we don't really drop anything, we just rearrange.
For me it is rather a question of spirit: Right now everything is very flexible and loose, your proposal is more structured and serious: The user needs to plan his program beforehand (What variables do I need in my session?) and then stick to this structure. That is not necessarily bad, it also helps to avoid bugs and so on, but it feels not as playful anymore and I wouldn't want to make this project Java-flavoured.
So I can't yet decide whether I like it or not, but this is a massive design choice and we should give it some thought first (and pay special attention to what is best for the user).

@crisward
Copy link
Contributor Author

crisward commented Mar 3, 2017

Thanks for considering this. I know what you mean about the j-word thing... however the schema does give you one place to look when you want to see what is being stored. It makes it self documenting and enables you to have default values etc.

Good luck with the decision...

@Thyra
Copy link
Contributor

Thyra commented Mar 4, 2017

Do you think there is a reasonable way to switch to this new structure while somewhat keeping backward compatibility? I'm thinking a default schema where primitive arrays are already defined (so that session.ints["foobar"] works out of the box) and the user can extend or replace the schema if he wants to use a more structured approach. That way we can provide all of the benefits of your idea but it is also possible to use kemal-session the traditional way without ever having to learn what a schema is.

@crisward
Copy link
Contributor Author

crisward commented Mar 4, 2017

That makes perfect sense.

@carcinocron
Copy link

my sessions table looks like this lol:

            session_id            |                                   data                                    |         updated_at         
----------------------------------+---------------------------------------------------------------------------+----------------------------
 e8909dff65e214892a1b5076bc55c56f | {"ints":{},"bigints":{},"strings":{},"floats":{},"bools":{},"objects":{}} | 2021-12-08 16:07:29.339638
 fe6137cdbf7fd3ad58195f47350e3f2c | {"ints":{},"bigints":{},"strings":{},"floats":{},"bools":{},"objects":{}} | 2021-12-09 15:41:22.388829
 610097aed2ad2ba00afcbfc392996312 | {"ints":{},"bigints":{},"strings":{},"floats":{},"bools":{},"objects":{}} | 2021-12-08 16:07:30.466987
 0619333b8f70702fa4bf7d53f343d18a | {"ints":{},"bigints":{},"strings":{},"floats":{},"bools":{},"objects":{}} | 2021-12-09 16:01:19.181023
 a09aca9cb4cf4cd2b989786c0f2a74c6 | {"ints":{},"bigints":{},"strings":{},"floats":{},"bools":{},"objects":{}} | 2021-12-08 16:07:31.886765

I would like a "define your own storage object" version. Maybe the {"ints":{},"bigints":{},"strings":{},"floats":{},"bools":{},"objects":{}} should be moved to an optional macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants