[GitHub] [airflow] potiuk commented on a change in pull request #9947: [GH-9708] Add type annotation to providers/jenkins

[GitBox <gi...@apache.org>]

potiuk commented on a change in pull request #9947:
URL: https://github.com/apache/airflow/pull/9947#discussion_r459280204



##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -235,3 +238,4 @@ def execute(self, context):
             # If we can we return the url of the job
             # for later use (like retrieving an artifact)
             return build_info['url']
+        return None

Review comment:
       It's a good practice to return a value consistently. If you return something in one place and then do not return anything at the end, it's a very likely case that you forgot the return.
   
   It is pure manifestation of one of the [Python's Zen principles](https://www.python.org/dev/peps/pep-0020/) - "Explicit is better than implicit"

##########
File path: airflow/providers/jenkins/example_dags/example_jenkins_job_trigger.py
##########
@@ -44,7 +44,7 @@
     job_trigger = JenkinsJobTriggerOperator(
         task_id="trigger_job",
         job_name="generate-merlin-config",
-        parameters={"first_parameter": "a_value", "second_parameter": "18"},
+        parameters='{"first_parameter": "a_value", "second_parameter": "18"}',

Review comment:
       I think it's because of the wrong type specified in the docstring (and yes it should not be changed here)
   

##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -94,11 +99,12 @@ class JenkinsJobTriggerOperator(BaseOperator):
 
     @apply_defaults
     def __init__(self,
-                 jenkins_connection_id,
-                 job_name,
-                 parameters="",
-                 sleep_time=10,
-                 max_try_before_job_appears=10,
+                 jenkins_connection_id: str,
+                 job_name: str,
+                 parameters: str = "",

Review comment:
       Actually, when you look at the "build_job_url" method code and docstring, it should be Optional[Union[str, Dict, List]]

##########
File path: airflow/providers/jenkins/operators/jenkins_job_trigger.py
##########
@@ -56,7 +60,7 @@ def jenkins_request_with_headers(jenkins_server, req):
         # Jenkins's funky authentication means its nigh impossible to distinguish errors.
         if e.code in [401, 403, 500]:
             raise JenkinsException(
-                'Error in request. Possibly authentication failed [%s]: %s' % (e.code, e.msg)
+                'Error in request. Possibly authentication failed [%s]: %s' % (e.code, e.reason)

Review comment:
       Nice!




----------------------------------------------------------------
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.

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