Re: [PR] feat: Implement using Playwright for taking screenshots in reports [superset]

["villebro (via GitHub)" <gi...@apache.org>]

villebro commented on code in PR #25247:
URL: https://github.com/apache/superset/pull/25247#discussion_r1343086622


##########
setup.py:
##########
@@ -183,6 +183,7 @@ def get_git_sha() -> str:
         ],
         "oracle": ["cx-Oracle>8.0.0, <8.1"],
         "pinot": ["pinotdb>=0.3.3, <0.4"],
+        "playwright": ["playwright==1.37.0"],

Review Comment:
   I think we try to avoid hard pinning in `setup.py` to minimize the risk of conflicts:
   ```suggestion
           "playwright": ["playwright=>1.37.0, <2"],
   ```



##########
requirements/base.txt:
##########
@@ -231,6 +231,8 @@ parsedatetime==2.6
     # via apache-superset
 pgsanity==0.2.9
     # via apache-superset
+playwright==1.37.0
+    # via apache-superset

Review Comment:
   The fact that it's marked as an optional dependency in `setup.py` IMO means it should not be in `base.txt`. Was this entry added by hand, or via `pip-compile-multi`? I assume we'd need to change `requirements/base.in` as follows to autogenerate this:
   
   ```
   -e file:.[playwright]
   ```
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org