Misc improvements mostly Jenkins #52

spbnick · 2018-05-28T07:27:38Z

These are changes I made for #51, but since I don't have time to finish that now, I'm separating them so they don't rot.

veruu

Thanks @spbnick , functionality-wise this looks good to me. I have some smaller requests, but they should be really easy to implement and then we can merge this :)

veruu · 2018-05-28T12:15:13Z

sktm/__init__.py

-    PUBLISH_FAILURE = 3
-    TEST_FAILURE = 4
-    BASELINE_FAILURE = 5
+import sktm.misc


Can you please move the import above sktm.patchwork so we keep the alphabetical order withing the categories?

Sure, fixing.

veruu · 2018-05-28T12:21:36Z

sktm/jenkins.py

+        Result:
+            The URL of the build result.
+        """
+        return "%s/job/%s/%s" % (self.server.base_server_url(), self.name,


Unrelated to your pull && note to self, this should be changed to use path joins instead of hardcoded slashes.

veruu · 2018-05-28T12:37:53Z

sktm/jenkins.py

-class skt_jenkins(object):
-    """Jenkins interface"""
-    def __init__(self, url, username=None, password=None):
+class Project(object):


Can we rename this to JenkinsProject or something similar, so it's clear it's a Jenkins project and not for example Patchwork project? I get that it's in jenkins.py file, but if you type from jenkins import Project and the information is lost from plain sight.

I agree that it is confusing if you import the Project alone into an unrelated module. However, if you don't deal with Jenkins directly in the module you're importing it into like that, you shouldn't really be doing it. If you really have to, you can write from sktm.jenkins import Project as JenkinsProject.

I think adding Jenkins to the class name would be against the idea of Python's packaging system. I think it would be better to then just use import sktm.jenkins as jenkins and write jenkins.Project. Otherwise, as depth of hierarchy grows we can get to silly levels of duplication. We can already, kind of, write SktmJenkinsProject.

If we can guarantee that it will be used in a way you mention here than it's fine I guess. I was just thinking about possibility of supporting different things in the future which might use similar names which would be confusing, since we already have several different things which are called 'project'.

I think we can enforce that in our reviews, and I assume we would anyway. I think each module is responsible for taking care of ensuring that things make sense within its own namespace. One way is to simply always use global, fully-qualified identifiers, but if you'd like to have shorter names then something like this can be used:

import sktm.jenkins as jenkins import sktm.patchwork as patchwork jp = jenkins.Project() pp = patchwork.Project()

Readability wins, so this works 👍 (except the nondescriptive two-letter variable names, but we aren't talking about those here)

veruu · 2018-05-28T12:42:19Z

sktm/jenkins.py

            username:    Jenkins user name.
            password:    Jenkins user password.
        """
+        # Name of the Jenkins project to operate on


This is a total nitpick, but do we really need to duplicate the comment that is present only few lines above in the docstirng?

Yeah, it looks wasteful. What I'm trying to do here is to document member variables, which can't have docstrings (unfortunately). Technically, constructor arguments and instance member variables don't have to be the same, and I'm simply trying to be consistent here (or stubborn, if you wish), even though it is duplication most of the time. That's also why I'm explicitly initializing some member variables only to have them overwritten with an expression or a function call sometimes, just to separate member documentation and a comment about how something is being done, while also assigning to that member.

I think documenting member variables is important, but we can discuss that. Or if you'd like another way to do it (e.g. some exceptions), I'll be glad to hear that :)

I totally with having documentation for attributes if a function call, class initialization etc is involved. But if the relation between __init__'s parameter and the attribute is a simple assignment (and they are even named the same on top of that), the comments don't add any value. They should offer some additional explanation and in this case they don't. Does that distinction make sense?

Yeah, they don't offer any additional explanation, I agree. I'll drop this, no problem.

What I'm targeting here is the case where you want to lookup what particular member variable is, when e.g. reading a source code of another member function. Python doesn't provide a simple way of locating them, and I'm trying to make a substitution - a place you can always look (somewhere at the start of __init__), where you can see all member variables declared and described, without having to read anything else and avoiding extra steps such as checking if the assignment is coming from __init__ arguments and then reading its docstring. However, given our other problems, this is insignificant.

If you check the "block and inline comments" part of the Google Python guide which we link, it says something similar: to describe things that aren't clear and not obvious ones (which is also my rule of thumb for commenting).

I totally agree with having everything declared in __init__ (and if we are creating new attributes outside of __init__ it should be fixed). As for the description, variable names should be descriptive enough, and only things that aren't obvious should warrant a comment. Would fixing the declarations at least partially solve the problem you have?

@veruu, yeah having some rules and consistency for member variables would make it much better, thank you :)

I think naming things well is absolutely very important, and I agree that good code should need little comments. However, once we start documenting interfaces/structures/etc. (as opposed to algorithms/code in general), I think it's easier to just document everything (i.e. every module/class/method/variable/etc.), both when writing (no thinking needed whether to document or not), and when reading (just keep reading comments, everything is there, no need to switch to code, plus a comment is a confirmation you got it right).

This is just my view, and my preferred way of doing things, we don't have to follow it :)

veruu · 2018-05-28T12:54:02Z

sktm/jenkins.py

        """
+        # Name of the Jenkins project to operate on
+        self.name = name
+        # Jenkins server interface


Again nitpicking, but I believe it would be more useful to say Initialize Jenkins server interface since it's not a simple assignment of a single value.

Weeeeelll, I agree that a constructor can be far from trivial, but ideologically, this line is just creating a value of this type and everybody should know how it works (i.e. __init__ is called and so on) and where to look for documentation (i.e. the class docstring and docstring for __init__ of that class).

So for me here it's again a matter of documenting a member variable, and documenting what's being done in the constructor. This is borderline, so if my explanation above is not sufficient, how about I split this into this:

# Jenkins server instance self.server = None

And this:

# Create Jenkins server instance # TODO Add support for CSRF protection self.server = jenkinsapi.jenkins.Jenkins(url, username, password)

I mean I'm just trying to document a member variable here, while also creating a class instance and not going into details about what the latter entails.

This kind of goes with how I view the attributes comments: The second part of what you write is IMHO sufficient. Double assignment isn't something that makes sense (unless it's only the initialization and the assignment of the value / class instance happens in different method).

My only point about the previous state was that it didn't mention anything about server instance creation itself and implied a simple assignment. We can discuss the conventions and our reasons for them live tomorrow so for better understanding :)

No problem, I'll change the comment to your version.

This is, again, just me trying to make a table of all member variables with descriptions, provide a way to quickly lookup what they mean, and separate it from the way they might be assigned. This is not that important, and we can home in onto a solution later, if we find something like this necessary.

veruu · 2018-05-28T13:28:48Z

sktm/jenkins.py

+            buildid:    Jenkins build ID to get the status of.
+
+        Return:
+            True if the build is complete.


We should mention else case here too, especially since by default None is returned, which is not the same thing as False.

I'm adding , False if not, but I don't see how None could be returned. I thought not can only result in True or False, or have I misunderstood something?

Yes, but it's not clear from the docstring. Based on it, it can be a if complete return True and nothing else, and in case the condition wouldn't pass None would be returned as a default. So it's an inconsistency between the docstring and code. Adding , False if not describes what happens much better :)

spbnick

Thank you, Veronika! I updated the PR and responded to your comments :)

spbnick · 2018-05-28T16:19:29Z

sktm/__init__.py

-    PUBLISH_FAILURE = 3
-    TEST_FAILURE = 4
-    BASELINE_FAILURE = 5
+import sktm.misc


Sure, fixing.

spbnick · 2018-05-28T16:32:06Z

sktm/jenkins.py

-class skt_jenkins(object):
-    """Jenkins interface"""
-    def __init__(self, url, username=None, password=None):
+class Project(object):


I agree that it is confusing if you import the Project alone into an unrelated module. However, if you don't deal with Jenkins directly in the module you're importing it into like that, you shouldn't really be doing it. If you really have to, you can write from sktm.jenkins import Project as JenkinsProject.

I think adding Jenkins to the class name would be against the idea of Python's packaging system. I think it would be better to then just use import sktm.jenkins as jenkins and write jenkins.Project. Otherwise, as depth of hierarchy grows we can get to silly levels of duplication. We can already, kind of, write SktmJenkinsProject.

spbnick · 2018-05-28T16:38:02Z

sktm/jenkins.py

            username:    Jenkins user name.
            password:    Jenkins user password.
        """
+        # Name of the Jenkins project to operate on


Yeah, it looks wasteful. What I'm trying to do here is to document member variables, which can't have docstrings (unfortunately). Technically, constructor arguments and instance member variables don't have to be the same, and I'm simply trying to be consistent here (or stubborn, if you wish), even though it is duplication most of the time. That's also why I'm explicitly initializing some member variables only to have them overwritten with an expression or a function call sometimes, just to separate member documentation and a comment about how something is being done, while also assigning to that member.

I think documenting member variables is important, but we can discuss that. Or if you'd like another way to do it (e.g. some exceptions), I'll be glad to hear that :)

spbnick · 2018-05-28T16:49:56Z

sktm/jenkins.py

        """
+        # Name of the Jenkins project to operate on
+        self.name = name
+        # Jenkins server interface


Weeeeelll, I agree that a constructor can be far from trivial, but ideologically, this line is just creating a value of this type and everybody should know how it works (i.e. __init__ is called and so on) and where to look for documentation (i.e. the class docstring and docstring for __init__ of that class).

So for me here it's again a matter of documenting a member variable, and documenting what's being done in the constructor. This is borderline, so if my explanation above is not sufficient, how about I split this into this:

# Jenkins server instance self.server = None

And this:

# Create Jenkins server instance # TODO Add support for CSRF protection self.server = jenkinsapi.jenkins.Jenkins(url, username, password)

I mean I'm just trying to document a member variable here, while also creating a class instance and not going into details about what the latter entails.

spbnick · 2018-05-28T16:58:42Z

sktm/jenkins.py

+            buildid:    Jenkins build ID to get the status of.
+
+        Return:
+            True if the build is complete.


I'm adding , False if not, but I don't see how None could be returned. I thought not can only result in True or False, or have I misunderstood something?

spbnick

Thank you, Veronika. Commented and updated!

spbnick · 2018-05-29T08:08:07Z

sktm/jenkins.py

-class skt_jenkins(object):
-    """Jenkins interface"""
-    def __init__(self, url, username=None, password=None):
+class Project(object):


I think we can enforce that in our reviews, and I assume we would anyway. I think each module is responsible for taking care of ensuring that things make sense within its own namespace. One way is to simply always use global, fully-qualified identifiers, but if you'd like to have shorter names then something like this can be used:

import sktm.jenkins as jenkins import sktm.patchwork as patchwork jp = jenkins.Project() pp = patchwork.Project()

spbnick · 2018-05-29T08:14:43Z

sktm/jenkins.py

            username:    Jenkins user name.
            password:    Jenkins user password.
        """
+        # Name of the Jenkins project to operate on


Yeah, they don't offer any additional explanation, I agree. I'll drop this, no problem.

What I'm targeting here is the case where you want to lookup what particular member variable is, when e.g. reading a source code of another member function. Python doesn't provide a simple way of locating them, and I'm trying to make a substitution - a place you can always look (somewhere at the start of __init__), where you can see all member variables declared and described, without having to read anything else and avoiding extra steps such as checking if the assignment is coming from __init__ arguments and then reading its docstring. However, given our other problems, this is insignificant.

spbnick · 2018-05-29T08:16:49Z

sktm/jenkins.py

        """
+        # Name of the Jenkins project to operate on
+        self.name = name
+        # Jenkins server interface


No problem, I'll change the comment to your version.

This is, again, just me trying to make a table of all member variables with descriptions, provide a way to quickly lookup what they mean, and separate it from the way they might be assigned. This is not that important, and we can home in onto a solution later, if we find something like this necessary.

Move sktm.tresult to sktm.misc.tresult to fix composition, so that modules lower in the hierarchy (e.g. sktm.jenkins) do not have to import the module above (i.e. sktm). Fixes cki-project#6

Instead of creating and using a "Jenkins Interface" (sktm.jenkins.skt_jenkins), and then supplying a project name ("jobname") with every method call, create and use a "Jenkins Project Interface" (sktm.jenkins.Project) and only supply the project name on its creation. This simplifes the interface, and removes a bit of code. This also prepares for abstracting Jenkins away.

Mark sktm.jenkins.Project private methods by prepending them with double underscore. Concerns cki-project#12

coveralls · 2018-06-05T09:05:05Z

Pull Request Test Coverage Report for Build 207

31 of 69 (44.93%) changed or added relevant lines in 6 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.01%) to 53.822%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sktm/patchwork.py	1	3	33.33%
sktm/init.py	3	12	25.0%
sktm/jenkins.py	15	42	35.71%

Files with Coverage Reduction	New Missed Lines	%
sktm/init.py	1	26.49%
sktm/jenkins.py	2	16.1%

Totals
Change from base Build 204:	-0.01%
Covered Lines:	683
Relevant Lines:	1269

💛 - Coveralls

Spell out "patch URL list" instead of writing "patchwork" in sktm.jenkins.Project, to make clear what's being accepted/returned. Leave updating the Jenkins project parameter name for later.

Create a complete Jenkins project interface instance and pass it to the watcher, instead of having the watcher create it. This prepares the watcher to using abstract scheduler interface, instead of Jenkins specifically.

spbnick · 2018-06-05T09:42:19Z

Alright, rebased and renamed sktm.jenkins.Project to sktm.jenkins.JenkinsProject. @veruu, could you please take a look again? @major, I'm also updating the tests in "Create Jenkins project interface outside watcher", could you check if it makes sense, please? Thank you!

spbnick · 2018-07-12T16:09:53Z

@danghai, do you think you can take this PR on, get it rebased, resubmitted and merged?

danghai · 2018-07-13T02:38:35Z

@spbnick yeah sure, let me look at it. You want me fetch your branch, fix it, and create another PR?

spbnick · 2018-07-13T07:26:51Z

@danghai, yes :)

danghai · 2018-07-14T08:07:40Z

I make the new PR for this #113. It does not finish yet. It is great if you put some comment.

spbnick · 2018-07-16T16:16:54Z

Superseded by #113.

spbnick requested a review from veruu May 28, 2018 07:27

veruu suggested changes May 28, 2018

View reviewed changes

spbnick force-pushed the misc_improvements_mostly_jenkins branch from 8d2e1c3 to 68e5047 Compare May 28, 2018 17:03

spbnick commented May 28, 2018

View reviewed changes

spbnick force-pushed the misc_improvements_mostly_jenkins branch from 68e5047 to cbc7367 Compare May 29, 2018 08:33

spbnick commented May 29, 2018

View reviewed changes

spbnick mentioned this pull request May 29, 2018

Move KernelBuilder into its own file cki-project/skt-archived#159

Merged

spbnick added 4 commits June 5, 2018 10:36

Move tresult into sktm.misc

b1ca0a4

Move sktm.tresult to sktm.misc.tresult to fix composition, so that modules lower in the hierarchy (e.g. sktm.jenkins) do not have to import the module above (i.e. sktm). Fixes cki-project#6

Describe sktm.jenkins.get_result* functions

13d307f

Mark sktm.jenkins.Project private methods

bbc4d6f

Mark sktm.jenkins.Project private methods by prepending them with double underscore. Concerns cki-project#12

spbnick force-pushed the misc_improvements_mostly_jenkins branch from cbc7367 to 14086b8 Compare June 5, 2018 09:01

spbnick added 4 commits June 5, 2018 12:36

Spell out patch URL list in sktm.jenkins

57b67b6

Spell out "patch URL list" instead of writing "patchwork" in sktm.jenkins.Project, to make clear what's being accepted/returned. Leave updating the Jenkins project parameter name for later.

Specify patch order for patch_url_list args

51e3575

Create Jenkins project interface outside watcher

ad52f4f

Create a complete Jenkins project interface instance and pass it to the watcher, instead of having the watcher create it. This prepares the watcher to using abstract scheduler interface, instead of Jenkins specifically.

Document sktm.jenkins.Project.is_build_complete()

c5b64b0

spbnick force-pushed the misc_improvements_mostly_jenkins branch from 14086b8 to c5b64b0 Compare June 5, 2018 09:37

spbnick mentioned this pull request Jun 6, 2018

WIP: Remove jenkins requirement #51

Open

danghai mentioned this pull request Jul 14, 2018

Misc improvements mostly Jenkins #113

Merged

spbnick closed this Jul 16, 2018

Misc improvements mostly Jenkins #52

Misc improvements mostly Jenkins #52

Uh oh!

Conversation

spbnick commented May 28, 2018

Uh oh!

veruu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veruu May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spbnick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spbnick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 207

💛 - Coveralls

Uh oh!

veruu May 28, 2018 •

edited

Loading

coveralls commented Jun 5, 2018 •

edited

Loading

danghai commented Jul 14, 2018 •

edited

Loading