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

AUTO_INCREMENT #164

Closed
wants to merge 2 commits into from
Closed

AUTO_INCREMENT #164

wants to merge 2 commits into from

Conversation

KyGost
Copy link
Contributor

@KyGost KyGost commented Mar 9, 2021

Relates: #159

@KyGost
Copy link
Contributor Author

KyGost commented Mar 9, 2021

Quite yuck and not yet working for rows inserted without NULL but working nonetheless.

@panarch how do you feel about giving StoreMut to Execute::prepare and Row::new?

@panarch
Copy link
Member

panarch commented Mar 9, 2021

@KyGost

  1. Execute::prepare
    I actually think Execute::prepare is... not meaningful to keep, first intention was dividing Store uses and Store + StoreMut uses but now I'm not sure that is really meaningful.
    It might be better to re-combine prepare and execute, how do you think?

  2. Row::new
    You mean that providing Store to Row::new? then I think it would be good to keep data modules not to depend on Store. making it lightweight, containing "data" and some useful utility methods.

@panarch panarch self-requested a review March 9, 2021 05:34
@panarch panarch added the enhancement New feature or request label Mar 9, 2021
@KyGost
Copy link
Contributor Author

KyGost commented Mar 9, 2021

@panarch

  1. Execute::prepare
    I actually think Execute::prepare is... not meaningful to keep, first intention was dividing Store uses and Store + StoreMut uses but now I'm not sure that is really meaningful.
    It might be better to re-combine prepare and execute, how do you think?

That sounds like a good idea.

  1. Row::new
    You mean that providing Store to Row::new? then I think it would be good to keep data modules not to depend on Store. making it lightweight, containing "data" and some useful utility methods.

Yup. I agree, it felt off to give it. How would you suggest it be done?
I need to run a tree.get & tree.set for each row.

@KyGost KyGost closed this Mar 12, 2021
@KyGost KyGost mentioned this pull request Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants