New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract and display project activity metrics #150

Merged
merged 13 commits into from Jul 25, 2017

Conversation

Projects
3 participants
@agathver
Copy link
Collaborator

agathver commented Jun 16, 2017

Displays and extracts metrics after git clone.

  • Extract commits
  • Extract contributors
  • Display commit count
  • Display contributors
  • Histograms for commits
  • Extract lines of code and comments
  • Display LOC
  • Tests
  • Code-style fixes

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from 411e5f4 to 5e3e03a Jun 16, 2017

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from 3c07d99 to ce394e3 Jun 19, 2017

agathver added some commits Jun 16, 2017

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from 56600e8 to b4529b8 Jun 20, 2017

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from b4529b8 to f6c8be1 Jun 21, 2017

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from 9144d37 to 1517381 Jun 23, 2017

agathver added some commits Jun 25, 2017

Fix various nits
1. Entity mappings
2. phpDoc commenta
3. Code-style

@agathver agathver removed the not yet ready label Jun 30, 2017

@@ -47,7 +47,7 @@ class Project
* @var string
*
* @Assert\Choice(choices = {"ASSIGNED", "UNASSIGNED"})
* @ORM\Column(type="string")
* @ORM\Column(type="string", options={"default" : self::STATUS_ASSIGNED})

This comment has been minimized.

@imphil

imphil Jul 3, 2017

Contributor

That's causing the following error during provisioning:

An error occurred when executing the \"'cache:clear --no-warmup'\" command:                                                                                                                                                                                                                                               ", "  PHP Fatal error:  Uncaught Doctrine\\Common\\Annotations\\AnnotationException: [Semantical Error] Couldn't find constant self::STATUS_ASSIGNED, property Librecores\\ProjectRepoBundle\\Entity\\Project::$status. in /var/www/lc/site/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php:54  ", "  Stack trace:                                                                                                                                                                                                                                                                                                            ", "  #0 /var/www/lc/site/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php(101): Doctrine\\Common\\Annotations\\AnnotationException::semanticalError('Couldn't find c...')                                                                                                                    ", "  #1 /var/www/lc/site/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php(973): Doctrine\\Common\\Annotations\\AnnotationException::semanticalErrorConstants('self::STATUS_AS...', 'property Librec...')                                                                                               ", "  #2 /var/www/lc/site/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php(1039): Doctrine\\Common\\Annotations\\DocParser->Constant()                                                                                                                                                                  ", "  #3 /var/www/lc/site/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php(1161): Doctrine\\Common\\Annotations\\DocParser->PlainValue() in /var/www/lc/site/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php on line 54

This comment has been minimized.

@imphil

imphil Jul 3, 2017

Contributor

Switching to "ASSIGNED" instead of using a constant works.

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 3, 2017

It also seems this PR is missing a database migration to update the DB to the latest schema, including the added tables. Please add that and run

vagrant destroy
vagrant up

to test if the provisioning, including loading of the db fixtures, works as expected.

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from 3400d4a to 7731ce3 Jul 4, 2017

@agathver agathver moved this from In Progress to Review in Code Quality Metrics Implementation Jul 5, 2017

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 8, 2017

The deployment in Vagrant works now, thanks.
But I still don't get a fully working version: it seems the chart js library is not properly loaded. The developer console says:

01:31:52.740 TypeError: $chart.height(...).peity is not a function 1 optimsoc:227:5
	<anonym> http://www.librecores.devel/test2/optimsoc:227:5
	m.Callbacks/j http://www.librecores.devel/js/app_body_part_1.js:2:27304
	m.Callbacks/k.fireWith http://www.librecores.devel/js/app_body_part_1.js:2:28122
	.ready http://www.librecores.devel/js/app_body_part_1.js:2:29954
	J http://www.librecores.devel/js/app_body_part_1.js:2:30320

This is how it looks in Vagrant on my machine:
grafik

Also, the JS code using $chart as variable looks very much inspired by PHP code. JS variables don't need/use $ in the beginning.

Some other comments:

  • Why does it say it would take 12 months by 97 developers -- where are the 97 developers coming from? (since there have been 13 contributors, as shown below.)
  • Please don't directly render relative times, but use the absolute time (in UTC) and let JS make a relative time out of it. See https://github.com/librecores/librecores-web/blob/master/site/src/Librecores/PlanetBundle/Resources/views/Default/index.html.twig#L20 (and don't forget the tooltip code at the end of the page.)
  • The optimsoc example also clearly shows why it would be useful to group contributions not only by email address, but also by name. We left this explicitly out of scope, but please create another issue as enhancement to track this for the future.
  • The UI in the datasheet is pretty crowded at the moment, but we're already working on that in the next iterations. So that's fine for now.

I'll have a closer look at the code once the functionality works for me to make sure we don't waste time reviewing code which will change anyways.

@agathver

This comment has been minimized.

Copy link
Collaborator

agathver commented Jul 9, 2017

@imphil

it seems the chart js library is not properly loaded.

I cannot reproduce it. It is the part of app_body_1.js, I can verify that, and the file isn't missing from the commits. Looks like a browser cache issue to me. (I recall seeing expire: max in nginx config).

I'll add a cache-busting filter to Assetic, we will surely need it in production es

Why does it say it would take 12 months by 97 developers -- where are the 97 developers coming from? (since there have been 13 contributors, as shown below.)

It's a COCOMO estimation. Generally used to estimate the worth of a open-source project if developed by a traditional in-house development team. This states it would take 12 months for 97 developers to develop code equivalent to it. Although I actually wanted to express it in monetary value, 12 * 97 * avg. monthly wage

The optimsoc example also clearly shows why it would be useful to group contributions not only by email address, but also by name. We left this explicitly out of scope, but please create another issue as enhancement to track this for the future.

Opened #158

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 9, 2017

I cannot reproduce it. It is the part of app_body_1.js, I can verify that, and the file isn't missing from the commits. Looks like a browser cache issue to me. (I recall seeing expire: max in nginx config).

I'll add a cache-busting filter to Assetic, we will surely need it in production es

There is indeed a bug somewere on my setup, but its most likely not the caching of nginx. Instead, it seems the assets don't get regenerated properly:

~/src/librecores-web/site/web/js> ll
total 728
-rw-r--r-- 1 philipp users   7228 Apr 28  2016 8864e78.js
-rw-rw-r-- 1 philipp users 239184 Sep 28  2016 app_body.js
-rw-rw-r-- 1 philipp users 239184 Sep 28  2016 app_body_part_1.js
-rw-rw-r-- 1 philipp users   7228 Sep 28  2016 app_head.js
-rw-rw-r-- 1 philipp users   7228 Sep 28  2016 app_head_part_1.js
-rw-r--r-- 1 philipp users 236240 Apr 28  2016 f0e7c52.js

I'll look into it.

@oleg-nenashev
Copy link
Collaborator

oleg-nenashev left a comment

I suggest removing COCOMO at least from the default output. This model has nothing to do with iterative projects like open-source projects under active maintenance.

@imphil
Copy link
Contributor

imphil left a comment

Thanks Amitosh for this PR. I took a look at it and have added some comments in the code. In addition, a couple more general comments (I didn't repeat them all over the code):

  • (Design) The repository crawler class hierarchy needs more thought. Let's continue the discussion on that on the mailing list.

  • (Design) Regarding the Metadata* classes: what distinguishes project metadata from project data?

  • (Style) Please don't write phpdoc comments in the assumption of a hidden "[This function] ...". Example: don't write "Forces a reload of all project metadata", but "Force a reload of all project metadata".

  • (Style) Make sure you have a newline at the end of the file.

  • (Style) Try harder to find descriptive names, and use them consistently.

@@ -145,9 +147,54 @@ public function viewAction(Request $request, $parentName, $projectName)
return $response;
}
// fetch project metadata
$manager = $this->get('librecores.repository_metadata_manager');

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

Please rename $manager to something more descriptive.

use Librecores\ProjectRepoBundle\Repository\ContributorRepository;
/**
* Implementation of MetadataManagerInterface

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

Could you be more descriptive in the comment here? What does this implementation actually do?

/**
* Implementation of MetadataManagerInterface
*/
class DefaultMetadataManager implements MetadataManagerInterface

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

I'm not convinced of all the "Metadata" names. Metadata of what? How about ProjectMetadata or something similar?

public function __construct(
CommitRepository $commitRepository,
ContributorRepository $contributorRepository
) {

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

style: we use

public function __construct(CommitRepository $commitRepository, 
                            ContributorRepository $contributorRepository) 
{

This comment has been minimized.

@agathver

agathver Jul 18, 2017

Collaborator

This is PSR-2 . Should I change it ?

use PDO;
/**
* Hydrates database results into a single column

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

style: s/Hydrates/Hydrate

content: what are "database results"? I don't get the description.

$(function(){
$('[data-toggle="tooltip"]').tooltip();
var $chart = $('.librecores-project-activity-graph');
$chart.height('100px').peity('line', { height: $chart.height(), width: $chart.width() });

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

Please don't use $chart as variable name in JS, it's not PHP.

* {@inheritDoc}
* @see RepoCrawler::commitExists()
*/
public function commitExists(string $id) : bool

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

$id => $commitId?

default: // anything other than 0 or 1 is error
//die($code);
throw new \RuntimeException("Unable to fetch commits from $cwd : ".$errors);
}

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

This switch statement needs refactoring. "// anything other than 0 or 1 is error" doesn't match reality, and "return $code === 0;" together with the fall-through case statements is hard to understand. Spend a line more code to make it readable.

/**
* @var SourceCrawlerInterface[]
*/
protected $sourceCrawlers;

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

Now thats rather confusing. We have a RepoCrawler which itself has SourceCrawlers? How are the tasks between the two separated, and how can this be reflected in the class/interface names?

This class is now (and partially has been) a mix between an actor and an entity representing an repository. Let's continue the discussion on the mailing list where you already came up with a first solution.

*
* @author Amitosh Swain Mahapatra <amitosh.swain@gmail.com>
*/
class Dates

This comment has been minimized.

@imphil

imphil Jul 17, 2017

Contributor

Can we find a more descriptive class name here?

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from abbaf2e to 1c79f1d Jul 19, 2017

@agathver

This comment has been minimized.

Copy link
Collaborator

agathver commented Jul 19, 2017

@oleg-nenashev I have disabled COCOMO metrics display

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch 6 times, most recently from 31fe7b5 to 7e53715 Jul 19, 2017

@agathver agathver force-pushed the agathver:gsoc_basic_repo_data branch from 7e53715 to 2ea9aca Jul 22, 2017

@agathver

This comment has been minimized.

Copy link
Collaborator

agathver commented Jul 22, 2017

@imphil @oleg-nenashev I have fixed the issues, please review again

@imphil

imphil approved these changes Jul 25, 2017

@imphil imphil merged commit 6345861 into librecores:master Jul 25, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 25, 2017

Merged, thanks @agathver. I've seen a couple minor things, but let's do that in subsequent iterations.

@oleg-nenashev

This comment has been minimized.

Copy link
Collaborator

oleg-nenashev commented Jul 25, 2017

I've seen a couple minor things, but let's do that in subsequent iterations.

Fine for me

@agathver agathver moved this from Review to Done in Code Quality Metrics Implementation Aug 9, 2017

@agathver agathver deleted the agathver:gsoc_basic_repo_data branch Aug 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment