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

Add python_version marker constraint in setup.py #367

Closed
s3rius opened this issue Oct 7, 2021 · 14 comments · Fixed by #368
Closed

Add python_version marker constraint in setup.py #367

s3rius opened this issue Oct 7, 2021 · 14 comments · Fixed by #368
Labels
bug Something isn't working

Comments

@s3rius
Copy link
Contributor

s3rius commented Oct 7, 2021

Describe the bug
I've found a dependency resolution problem when installing ormar with black using poetry.

  Because no versions of black match >21.9b0,<22.0
   and black (21.9b0) depends on typing-extensions (>=3.10.0.0), black (>=21.9b0,<22.0) requires typing-extensions (>=3.10.0.0).
  And because ormar (0.10.20) depends on typing-extensions (>=3.7,<3.10.0.3)
   and no versions of ormar match >0.10.20,<0.11.0, black (>=21.9b0,<22.0) is incompatible with ormar (>=0.10.20,<0.11.0).
  So, because ormartest depends on both ormar (^0.10.20) and black (^21.9b0), version solving failed.

To Reproduce
Steps to reproduce the behavior:

  1. poetry new test
  2. cd test
  3. poetry add black --dev
  4. poetry add ormar
  5. You found the error.

Expected behavior
A clear and concise description of what you expected to happen.

Versions (please complete the following information):

  • Database backend used (mysql/sqlite/postgress)
  • Python version
  • poetry ^1.1.11
  • ormar ^0.10.20
  • black ^21.9b0

Possible solution

Since you use typing_extensions only for providing Protocol and Literal, you may add constraint to avoid this issue.
The solution is to specify the typing_extension version in your setup.py as following:

install_requires=[
        ...
        "typing_extensions>=3.7,<3.10.0.3; python_version < '3.8'",
    ],

Additional context

This issue is crucial for me, since I want to integrate your super cool library in my project generator. It uses black by default, but I can't get it work with your library because of this issue.

Thanks in advance.

@s3rius s3rius added the bug Something isn't working label Oct 7, 2021
@s3rius
Copy link
Contributor Author

s3rius commented Oct 7, 2021

I can make pull request that adds this constraint or I can add poetry as your dependency manager. Poetry would be really nice because it's easier to maintain than setup.py for both development and publication.

@s3rius
Copy link
Contributor Author

s3rius commented Oct 7, 2021

I've created a workaround in my project so this issue is not so important for me, but for someone it might be useful.

@collerek
Copy link
Owner

collerek commented Oct 7, 2021

If you could issue a PR pinning the python version that would be awesome! ☺️

As for the poetry i was thinking about it for some time already, i use poetry in other projects and i like it. So if you are willing to make the necessary changes and issue another pr (so i can test deploys etc) that would be even more awesome! 😁

@s3rius
Copy link
Contributor Author

s3rius commented Oct 7, 2021

Ok. The I'll try to make it in the best way possible. 🌟 Thanks you for your fast reply. 🌟

@s3rius
Copy link
Contributor Author

s3rius commented Oct 7, 2021

I've got some questions.

I've created pyproject.toml with minimum python version "^3.6.2" is it ok?

Also I faced an issue that black formats code and flake finds errors.
Would you mind if I'll fix them?

Here is the log of my actions with output.
https://pastebin.com/9zr9EBNr

@collerek
Copy link
Owner

collerek commented Oct 8, 2021

Yes, up 3.6.2 if fine.

As for the flake8 check the config in .flake8 file I specifically disabled some checks.

The amount of black reformatted files seems suspicious too, what is the line length setting? (should be 88)

@s3rius
Copy link
Contributor Author

s3rius commented Oct 8, 2021

I haven't change the any config file.

Here is some diffs with before and after black:

-def post_save(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable:
+def post_save(
+    senders: Union[Type["Model"], List[Type["Model"]]],
+) -> Callable:
     """
     Connect given function to all senders for post_save signal.
 
@@ -54,7 +56,9 @@ def post_save(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable:
     return receiver(signal="post_save", senders=senders)
 
 
-def post_update(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable:
+def post_update(
+    senders: Union[Type["Model"], List[Type["Model"]]],
+) -> Callable:
     """
     Connect given function to all senders for post_update signal.
 
@@ -67,7 +71,9 @@ def post_update(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable
     return receiver(signal="post_update", senders=senders)
 
 
-def post_delete(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable:
+def post_delete(
+    senders: Union[Type["Model"], List[Type["Model"]]],
+) -> Callable:
     """
     Connect given function to all senders for post_delete signal.
 
@@ -43,8 +43,8 @@ class NoMatch(AsyncOrmException):
 
 class MultipleMatches(AsyncOrmException):
     """
-        Raised for database queries that should return one row (i.e. get, first etc.)
-        but has multiple matching results in response.
+    Raised for database queries that should return one row (i.e. get, first etc.)
+    but has multiple matching results in response.
     """
 
     pass
@@ -52,11 +52,11 @@ class MultipleMatches(AsyncOrmException):
 
 class QueryDefinitionError(AsyncOrmException):
     """
-        Raised for errors in query definition:
+    Raised for errors in query definition:
 
-        * using contains or icontains filter with instance of the Model
-        * using Queryset.update() without filter and setting each flag to True
-        * using Queryset.delete() without filter and setting each flag to True
+    * using contains or icontains filter with instance of the Model
+    * using Queryset.update() without filter and setting each flag to True
+    * using Queryset.delete() without filter and setting each flag to True
     """
 
     pass

I guess the game changer for function is a comma after the function parameters.

Like

def test(a: int,) -> None: # Would be refformated
   ...

def test(a: int) -> None: # Wouldn't be refformated
   ...

But why does it reformat docstring like this?

@collerek
Copy link
Owner

collerek commented Oct 8, 2021

I think string-related handling was experimental and was enabled by default in the newest version of black.

but changes like those seem weird as it was black which inserted those commas in the first place :)

-def post_save(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable:
+def post_save(
+    senders: Union[Type["Model"], List[Type["Model"]]],
+) -> Callable:
     """
     Connect given function to all senders for post_save signal.
 
@@ -54,7 +56,9 @@ def post_save(senders: Union[Type["Model"], List[Type["Model"]]],) -> Callable:
     return receiver(signal="post_save", senders=senders)

@s3rius
Copy link
Contributor Author

s3rius commented Oct 8, 2021

It's looks really strange.

@s3rius
Copy link
Contributor Author

s3rius commented Oct 8, 2021

You can provide the '-C" parameter to the black, in order to exclude magic commas and it will remove all trailing commas.

Like this:

+++ ormar/relations/querysetproxy.py	2021-10-08 09:26:28.502277 +0000
@@ -292,11 +292,11 @@
         :type exclude_through: bool
         :param fields: field name or list of field names to extract from db
         :type fields:  Union[List, str, Set, Dict]
         """
         return await self.queryset.values(
-            fields=fields, exclude_through=exclude_through,
+            fields=fields, exclude_through=exclude_through
         )
 
     async def values_list(
         self,
         fields: Union[List, str, Set, Dict] = None,
@@ -477,12 +477,11 @@
         children = await self.queryset.all()
         for child in children:
             await child.update(**kwargs)  # type: ignore
             if self.type_ == ormar.RelationType.MULTIPLE and through_kwargs:
                 await self.update_through_instance(
-                    child=child,  # type: ignore
-                    **through_kwargs,
+                    child=child, **through_kwargs  # type: ignore
                 )
         return len(children)
 
     async def get_or_create(self, *args: Any, **kwargs: Any) -> "T":
         """
would reformat ormar/relations/querysetproxy.py
--- ormar/queryset/queryset.py	2021-10-08 09:25:14.915843 +0000
+++ ormar/queryset/queryset.py	2021-10-08 09:26:28.599002 +0000
@@ -245,11 +245,11 @@
         :rtype: sqlalchemy.Table
         """
         return self.model_meta.table
 
     def build_select_expression(
-        self, limit: int = None, offset: int = None, order_bys: List = None,
+        self, limit: int = None, offset: int = None, order_bys: List = None
     ) -> sqlalchemy.sql.select:
         """
         Constructs the actual database query used in the QuerySet.
         If any of the params is not passed the QuerySet own value is used.
 
@@ -376,11 +376,11 @@
             rel._access_chain if isinstance(rel, FieldAccessor) else rel
             for rel in related
         ]
 
         related = sorted(list(set(list(self._select_related) + related)))
-        return self.rebuild_self(select_related=related,)
+        return self.rebuild_self(select_related=related)
 
     def select_all(self, follow: bool = False) -> "QuerySet[T]":
         """
         By default adds only directly related models.
 
@@ -402,11 +402,11 @@
         :rtype: Model
         """
         relations = list(self.model.extract_related_names())
         if follow:
             relations = self.model._iterate_related_models()
-        return self.rebuild_self(select_related=relations,)
+        return self.rebuild_self(select_related=relations)
 
     def prefetch_related(
         self, related: Union[List, str, FieldAccessor]
     ) -> "QuerySet[T]":
         """
@@ -432,11 +432,11 @@
             rel._access_chain if isinstance(rel, FieldAccessor) else rel
             for rel in related
         ]
 
         related = list(set(list(self._prefetch_related) + related))
-        return self.rebuild_self(prefetch_related=related,)
+        return self.rebuild_self(prefetch_related=related)
 
     def fields(
         self, columns: Union[List, str, Set, Dict], _is_exclude: bool = False
     ) -> "QuerySet[T]":
         """

@collerek
Copy link
Owner

collerek commented Oct 8, 2021

Yeah I see this is something quite new: psf/black#1288
So my guess would be:
-> run black with -C to remove the commas (if that is what is actually happening, not just ignoring them)
-> and later run black without -C flag, so you can explode the arguments list if you want to.

Or if you remove -C after first call black adds those commas back?

@s3rius
Copy link
Contributor Author

s3rius commented Oct 8, 2021

I ran black twice with "-C" and without "-C" flag. It won't add those command back.

@s3rius
Copy link
Contributor Author

s3rius commented Oct 8, 2021

Can you please check the request #368?

I can close it and modify setup.py if you wish.

@s3rius
Copy link
Contributor Author

s3rius commented Oct 8, 2021

The request #369 adds python_version marker as a fast fix to this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants