Skip to content

Minor fix for unnecessary operations. - #1

Merged
pombredanne merged 1 commit into
aboutcode-org:masterfrom
corona10:quick_fix
Mar 3, 2017
Merged

Minor fix for unnecessary operations.#1
pombredanne merged 1 commit into
aboutcode-org:masterfrom
corona10:quick_fix

Conversation

@corona10

@corona10 corona10 commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

For this weekend, I am going to read your repos.
Actually, this PR is not significant.
Just one thing I noticed, while I read your code.
There was an unnecessary operations which is to find the smallest pid.

If ts does not exist, it doesn't have to compare old one.

@corona10
corona10 force-pushed the quick_fix branch 2 times, most recently from 257079e to 923c2da Compare March 2, 2017 23:08
@pombredanne

Copy link
Copy Markdown
Member

@corona10 Thanks!

@pombredanne

Copy link
Copy Markdown
Member

were you able to run the tests locally alright btw?

@corona10

corona10 commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

@pombredanne Sure! Everything worked fine. It will be great if there are some install guide for altgraph,docopt by installed pip.

@pombredanne

Copy link
Copy Markdown
Member

@corona10 the only thing needed normally is to checkout a clone and run ./configure then . bin/activate
This will install all the deps in a virtualenv including the test deps. And fetches them all locally from the thirdparty directory ensuring reproducible envs.

@pombredanne

Copy link
Copy Markdown
Member

I will merge this is in as soon as a the CI tests pass.

@corona10

corona10 commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@pombredanne That's nice tips. Thanks!

@pombredanne

Copy link
Copy Markdown
Member

Merged/ Thanks!

@pombredanne
pombredanne merged commit f75a2d0 into aboutcode-org:master Mar 3, 2017
@pombredanne

Copy link
Copy Markdown
Member

Comment for future commits messages:

  1. we like to prefix the 1st line with an issue number. Here this was a trivial change so it did not warrant it. If it had one a you would have added #156 as a prefix to the first line.

  2. we like to tell what the commit is about and use an imperative style, like if you were giving an order to the codebase with your commit:

e.g rather than : Minor fix for unnecessary operations.
may be #123 Remove unnecessary operations.
or

#123 Remove unnecessary operations.

   * If the ts timestamp does not exist, do not compare 
     with old one.
  1. You need to add a signoff to your commit. So the final message would have looked like this:
#123 Remove unnecessary operations.

   * If the ts timestamp does not exist, do not compare 
     with old one.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

@pombredanne

Copy link
Copy Markdown
Member

I summarized our guidelines here: https://github.com/nexB/aboutcode/wiki/Writing-good-commit-messages

@corona10

corona10 commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@pombredanne Sounds great. Thank you for complete documentations. I will take a look at it and follow.

@corona10
corona10 deleted the quick_fix branch March 4, 2017 01:43
pombredanne pushed a commit that referenced this pull request Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants