Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Minor fixes #117

Merged
merged 6 commits into from
Jul 24, 2018
Merged

Minor fixes #117

merged 6 commits into from
Jul 24, 2018

Conversation

danghai
Copy link
Contributor

@danghai danghai commented Jul 20, 2018

I make PR to make minor fixes

@coveralls
Copy link

coveralls commented Jul 20, 2018

Pull Request Test Coverage Report for Build 398

  • 3 of 11 (27.27%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 37.933%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sktm/patchwork.py 2 4 50.0%
sktm/init.py 1 7 14.29%
Totals Coverage Status
Change from base Build 391: -0.04%
Covered Lines: 462
Relevant Lines: 1094

💛 - Coveralls

Copy link
Contributor

@veruu veruu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hai! I have one small request, but besides that, I'd prefer to change the watcher and patchwork class renaming. The watcher class has a comment about how it needs to be properly renamed or refactored, so only fixing the capitalization isn't very useful. Similarly, the patchwork classes should get similar names to their parent, to make it reflect that they are specific interfaces to the project, and the name doesn't need to include "skt" either.

I'd either drop those three patches from the pull, or come up with appropriate names which don't need to be changed next time. What do you think?

sktm/__init__.py Outdated
@@ -21,7 +21,6 @@
import enum

import sktm.db
import sktm.jenkins
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the imports in alphabetical order and remove the jenkins import below instead?

@danghai
Copy link
Contributor Author

danghai commented Jul 21, 2018

@veruu Thanks review. I do not come up with the name too. Do you have any suggestion for them. If not, I could probably drop them. What do you think?

@veruu
Copy link
Contributor

veruu commented Jul 23, 2018

For the watcher, I'd keep that one untouched for now. As you might have noticed from the other pulls we want to split job queuing and checking so in the end, the watcher may be simplified or split appropriately as well. For the Patchwork classes, what would you think about PatchworkV1Project and PatchworkV2Project, to note the version as well as the fact that it's the interface to the project and not whole Patchwork instance?

@danghai danghai force-pushed the sktm_enhance branch 2 times, most recently from 4a59c5b to 16be1a2 Compare July 24, 2018 14:33
@danghai
Copy link
Contributor Author

danghai commented Jul 24, 2018

@veruu I drop the watcher's patch. The class names is good too, I fixed it. You can review now.

Copy link
Contributor

@veruu veruu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hai, looks good!

@veruu veruu merged commit 6adbe2a into CKI-project:master Jul 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants