Cleanup and fixes

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?

[lemonyte]

Cleaned up and formatted the code, also fixed a couple broken tests.
Changes:

• Changed minimum supported Python version to 3.6 since f-strings and async yield are used
• Replaced type() check with isinstance() check
• Replaced asserts in main package code with proper exceptions
• Replaced .format() calls with f-strings since they are already used in other places
• Added host parameter to Base() and Drive() functions not in the Deta class
• Removed accidental unused import
• Removed trailing and blank line whitespace
• Fixed test command in CONTRIBUTING.md
• Fixed test_ttl test
• Added DETA_SDK_TEST_TTL_ATTRIBUTE to env.sample as seen in the pull request workflow
• Various other formatting and style changes using flake8
• Formatted using black -l 100 (same as Clean-Up #48)

What is the purpose of the Deta class in __init__.py? It doesn't seem to accomplish anything new that isn't already available when using the module directly, other than setting the project key and id manually. It's a little confusing having two almost identical sets of methods. Perhaps a better solution would be to have an init() function to manually set the project key and id.

[abdelhai]

hi @LemonPi314 thanks for the great PR!
what are your thought about the previous PRs we received? Worth reworking them in this PR?

[lemonyte]

what are your thought about the previous PRs we received? Worth reworking them in this PR?

I would be happy to do that, or if you prefer I could open a new PR. Any in particular you want me to take a look at?

[abdelhai]

@LemonPi314 thanks for the quick response! i feel all are worth taking a look at, you could help us decide which ones we should consider merging. one new monolithic PR would be awesome!

i'm assuming this/the new pr will be breaking and we'll have to release a major version. rn, maybe we could group that with async support... what do you think?

[lemonyte]

So far nothing in any of the PRs are breaking changes (type hints, formatting, and an added argument to the AsyncBase class).
I will integrate #14#68 and #70 into this PR.
As for async support, I do think that would be better off separate, since that may include some breaking changes.

[abdelhai]

i believe yours already does as you're raising a ValueError instead of other exceptions/errors.

[lemonyte]

That is true. In that case it may be worth changing the package behavior slightly as described in my original comment since it is a breaking change as well.

[lemonyte]

A few questions:

• For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?
• Should I add an argument for an external session here or wait until it gets resolved in its own PR?

[abdelhai]

the reason behind the Deta class is that you configure the Deta cloient once and you can use all the services. (on Micros, it's per-configured, hence why you can just import the service "class". It's a better devex imo:

# this is the original apifromdetaimportDetadeta=Deta(<mykey/futureconfig>)
users=deta.Base("users")
photos=deta.Drive("photos")
# this is just a convenience api that works on Micros OR if the project key is set in the environmentfromdetaimportBase, Driveusers=Base("users")
photos=Drive("photos")

We can require devs to put the project key in the env, but what happens if we need more config?

For type hints is typing.Optional preferred for all the optional arguments (where None is the default value)?

is this the pythonic way of doing it? haven't touch python code for a while.

Should I add an argument for an external session here or wait until it gets resolved in its own PR?

you are right, let's keep async separate. i need to think about the api design

[lemonyte]

is this the pythonic way of doing it?

It is good practice to explicitly specify that a parameter type can be None when its default value is a literal None. However it adds a bit of bloat to the code so if you're against it do let me know.
The slight difference shows up in editor tooltips.

Without Optional (original):

defBase(self, name: str, host: str=None):

With Optional:

defBase(self, name: str, host: typing.Optional[str] =None):

For methods where there are two mutually exclusive parameters, like expire_in and expire_at, I will add overloads for better clarity.

[abdelhai]

thanks for the clarification! I'm not seeing the benefit from typing.Optional[str], the first screenshot shows everything the developer needs to know to use the API. So, I would leave it out to reduce bloat.

[lemonyte]

Update:

• Improved type hints across all files
• Added overloads for some methods
• Minor refactoring changes
• Combined fetch() and _fetch() methods in _Base() class
• Minor changes in tests
• Updated scripts according to CONTRIBUTING.md

[lemonyte]

@abdelhai it seems the checks are failing due to missing project keys. Can you check the workflow secrets?