Skip to content
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

Create database entities and update the database #234

Closed
wants to merge 15 commits into from

Conversation

@sandipbhuyan
Copy link
Collaborator

commented May 14, 2018

For classification system #232 The database entities are created. These databases will help us to store data for the classification system
The ProjectClassifier entity will store classification categories for a project and the ClassificationHierarchy entity will store the Classification hierarchy information that helps a user to specify classifiers for IP Cores.

@imphil
Copy link
Contributor

left a comment

Hi @sandipbhuyan I've mentioned a couple remarks in the code itself. In addition a couple comments:

  • Please remove the "[TASK]" prefix from the commit messages, that's useless and just takes away space. Keep the summary line (the first line) of the commit message below 50 characters.

  • The ProjectClassifier is missing an association to the Project it classifies.

  • Please use the PHPdoc comments to actually document things where necessary. It helps greatly to add at least a line of documentation what variables within entities do if they are very obvious. This makes both review and reading the code much easier.

@@ -95,6 +96,12 @@ public function load(ObjectManager $manager)
$projectOptimsoc->setSourceRepo($sourcerepoOptimsoc);
$manager->persist($projectOptimsoc);
//classificatioin hierarchy

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

Style: space after //, and please fix the typo.

@@ -95,6 +96,12 @@ public function load(ObjectManager $manager)
$projectOptimsoc->setSourceRepo($sourcerepoOptimsoc);
$manager->persist($projectOptimsoc);
//classificatioin hierarchy
foreach ($this->classifiers as $arr) {

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

Can you use more self-describing names than $arr and $arr[0]?

* @param string $name
* @return \Librecores\ProjectRepoBundle\Entity\ClassificationHierarchy
*/
private function createClassifier($parent_id,$name) {

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

we use camelCase for variables. Add a space between the variables, and place the opening { in a new line.

return $classifier;
}
private $classifiers = array(

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor
  • Class variables should go to the start of the file.
  • This could also be a constant.
  • Introduce a space after the ,
  • Add a comment what the first and second element in the array are for.
* ClassificationHierarchy
*
* @ORM\Table(name="classification_hierarchy")
* @ORM\Entity(repositoryClass="Librecores\ProjectRepoBundle\Repository\ClassificationHierarchyRepository")

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

As your repository classes are empty anyways, can't you just use the doctrine default ones?

* @ORM\Table(name="project_classifier")
* @ORM\Entity(repositoryClass="Librecores\ProjectRepoBundle\Repository\ProjectClassifierRepository")
*/
class ProjectClassifier

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

This isn't a project classifier, which would be a tool or an application or an algorithm to classify a project. This is the classification that's assigned to a project. So something like "ProjectClassification" would be more appropriate.

*
* @ORM\Column(name="parent_id", type="integer")
*/
private $parentId;

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

what's that used for?

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Use auto-populated reference entities instead of directly accessing database foreign key references.

*
* @ORM\Column(name="parentUser_id", type="integer")
*/
private $parentUserId;

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

What's that used for?

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Same as before

*
* @ORM\Column(name="updated_at", type="datetime")
*/
private $updatedAt;

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

will you really update these entries, as opposed to deleting them and assigning new ones?

* ProjectClassifier
*
* @ORM\Table(name="project_classifier")
* @ORM\Entity(repositoryClass="Librecores\ProjectRepoBundle\Repository\ProjectClassifierRepository")

This comment has been minimized.

Copy link
@imphil

imphil May 15, 2018

Contributor

same as before: the repository is empty, so you can just use the default one.

@agathver
Copy link
Collaborator

left a comment

Please take care of the style guide.
We loosely follow the Symfony Coding Standard.

But we believe in readability first. It is acceptable to deviate for readability.

}
private $classifiers = array(
array(0,'Language'),

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Nit: Prefer [ ] instead of array()

*/
public function prePersist()
{
$this->createdAt = new \DateTime;

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Do we need this hook?

In other places, we directly assign new \DateTime() in the constructor.

*/
public function preUpdate()
{
$this->updatedAt = new \DateTime;

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

It may be useful to move these functions into an AutoTimestampUpdate trait.

*
* @ORM\Column(name="parent_id", type="integer")
*/
private $parentId;

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Use auto-populated reference entities instead of directly accessing database foreign key references.

*
* @ORM\Column(name="parentUser_id", type="integer")
*/
private $parentUserId;

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Same as before

*
* @return ProjectClassifier
*/
public function setParentId($parentId)

This comment has been minimized.

Copy link
@agathver
*
* @return int
*/
public function getParentId()

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Same as above. Deal with objects

* This class was generated by the Doctrine ORM. Add your own custom
* repository methods below.
*/
class ClassificationHierarchyRepository extends \Doctrine\ORM\EntityRepository

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Not needed. You can use the doctrine default repository

*
* This class was generated by the Doctrine ORM. Add your own custom
* repository methods below.
*/

This comment has been minimized.

Copy link
@agathver

agathver May 15, 2018

Collaborator

Not needed

@agathver

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2018

It appears that you use Git flow?

It does not make sense in an open source project. The "[TASK] / [FEATURE]" in the commit lines are not necessary.

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2018

@agathver I am amending the commits and updating the PR with the changes.

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:database-extension branch from f9c020b to 000daf8 May 15, 2018

foreach ($this->classifiers as $arr) {
$classifier = $this->createClassifier($arr[0], $arr[1]);
// classification hierarchy
foreach (self::classifiers as $category) {

This comment has been minimized.

Copy link
@agathver

agathver May 18, 2018

Collaborator

Why self:: ?

This comment has been minimized.

Copy link
@sandipbhuyan

sandipbhuyan May 18, 2018

Author Collaborator

For accessing the constant of that fixture class named as classifiers.

This comment has been minimized.

Copy link
@agathver

agathver May 19, 2018

Collaborator

👍

@@ -133,39 +133,39 @@ private function createUser($username, $password)
* @param string $name
* @return \Librecores\ProjectRepoBundle\Entity\ClassificationHierarchy
*/
private function createClassifier($parent_id,$name) {
private function createClassifier($parentId, $name)

This comment has been minimized.

Copy link
@agathver

agathver May 18, 2018

Collaborator

createClassificationHierachy

{
$this->parentUserId = $parentUserId;
$this->projectId = $projectId;

This comment has been minimized.

Copy link
@agathver

agathver May 18, 2018

Collaborator

You don't need project id, use association with Project directly.

* @ORM\OneToMany(targetEntity="ProjectClassification", mappedBy="parentUser")
* @ORM\JoinColumn(name="projectId", referencedColumnName="id")
*/
protected $projecClassification;

This comment has been minimized.

Copy link
@agathver

agathver May 18, 2018

Collaborator

Typo: $projectClassification

* @var ArrayCollection
*
* @ORM\OneToMany(targetEntity="ProjectClassification", mappedBy="parentUser")
* @ORM\JoinColumn(name="projectId", referencedColumnName="id")

This comment has been minimized.

Copy link
@agathver

agathver May 18, 2018

Collaborator

No need to explicitly define join column unless it is ambiguous, let Symfony manage however it seems fit.

@@ -130,6 +138,7 @@ public function __construct()
$this->projects = new ArrayCollection();
$this->organizationsCreated = new ArrayCollection();
$this->organizationMemberships = new ArrayCollection();
$this->projecClassification = new ArrayCollection();

This comment has been minimized.

Copy link
@agathver

agathver May 18, 2018

Collaborator

Typo again

@sandipbhuyan sandipbhuyan self-assigned this May 18, 2018

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:database-extension branch from add647e to 5be3879 May 18, 2018

*
* @ORM\Column(name="parent_id", type="integer")
*/
private $parentId;

This comment has been minimized.

Copy link
@agathver

agathver May 21, 2018

Collaborator

Use self-reference, many-to-one and one-to-many

*
* @return ClassificationHierarchy
*/
public function setParentId($parentId)

This comment has been minimized.

Copy link
@agathver

agathver May 21, 2018

Collaborator

Use getParent/setParent

* @ORM\ManyToOne(targetEntity="User", inversedBy="projectClassification")
* @ORM\JoinColumn(nullable=true, onDelete="SET NULL")
*/
private $parentUser;

This comment has been minimized.

Copy link
@agathver

agathver May 21, 2018

Collaborator

Do we need this?

*
* @return ProjectClassification
*/
public function setParentUser(User $parentUser = null)

This comment has been minimized.

Copy link
@agathver

agathver May 21, 2018

Collaborator

Not required

sandipbhuyan added some commits May 22, 2018

Add Association mapping to all entity files and create migration to …
…populate ClassificationHierarchy table

    Association mapping have been implemented among all entities and a migration file have been created to populate ClassificationHierarchy table
@imphil
Copy link
Contributor

left a comment

I've commented a couple more general things, and please also remove "dead code", i.e. commented out code.

* @ORM\OneToMany(targetEntity="ProjectClassification", mappedBy="project")
* @ORM\JoinColumn(name="projectId", referencedColumnName="id")
*/
protected $projects;

This comment has been minimized.

Copy link
@imphil

imphil May 22, 2018

Contributor

This item is named a bit misleadingly. Don't associate a project with another project, associate a project with a ProjectClassification entity. So "protected $classifications" would work.

*
* @ORM\Column(name="categories", type="text")
*/
private $categories;

This comment has been minimized.

Copy link
@imphil

imphil May 22, 2018

Contributor

This is a string representing a single category/classification. Make this clear by using a singular term, e.g. "classification", or "category" (preference given to "classification", since that's equivalent to the name of this entity).

* @var Project
*
* @ORM\ManyToOne(targetEntity="Project", inversedBy = "projects")
* @ORM\JoinColumn(nullable=true, onDelete="SET NULL")

This comment has been minimized.

Copy link
@imphil

imphil May 22, 2018

Contributor

SET NULL is not the right method here. If deleting a Project the ProjectClassification entities should also be deleted. If deleting a ProjectClassification nothing should happen to the Project.

/**
* A classification hierarchy for the projects
*
*It contains classification categories that can be use to classify the IP Cores

This comment has been minimized.

Copy link
@imphil

imphil May 22, 2018

Contributor

nit: * It (missing space)

// $this->addSql('INSERT INTO ClassificationHierarchy(parent_id,name,created_at,updated_at) VALUES (null,"License","2018-05-22 12:22:05","2018-05-22 12:22:05")');
foreach (self::classifier as $categories)

This comment has been minimized.

Copy link
@imphil

imphil May 22, 2018

Contributor

style: foreach () { (opening curly brackets on the same line)

This comment has been minimized.

Copy link
@agathver

agathver May 23, 2018

Collaborator

I think it will be better to move the migrations from #236 into this PR

Add onDelete condition on ManyToOne relation
Added on delete cascade feature to ProjectClassification Entity, changed the name of entity variables and modified the migration file

sandipbhuyan added some commits May 24, 2018

Add prepared statement syntax to migration file
Repace the null strings with NULL in migration and add prepared statement to it
@imphil

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

Thanks Sandip for the changes. However we need a bit more work on this one unfortunately.

First of all, I'm not able to create a Vagrant test VM with these changes. When setting up a new instance I get the following error:

fatal: [librecores]: FAILED! => {"changed": true, "cmd": ["php", "bin/console", "doctrine:migrations:migrate", "-n", "--no-ansi"], "delta": "0:00:02.255971", "end": "2018-05-28 14:51:01.150210", "msg": "non-zero return code", "rc": 1, "start": "2018-05-28 14:50:58.894239", "stderr": "14:51:01 ERROR     [console] Error thrown while running command \"doctrine:migrations:migrate -n --no-ansi\". Message: \"An exception occurred while executing 'INSERT INTO ClassificationHierarchy(parent_id,name,created_at,updated_at) VALUES (?,?,?,?)' with params [null, \"License\", \"2018-05-28 14:51:01\", \"2018-05-28 14:51:01\"]:\n\nSQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.ClassificationHierarchy' doesn't exist\"

(and so on)

Please see my comments below which also will solve this one.

  1. General.
    • Keep lines at 80 characters roughly. You don't need to spell out classes with all namespaces in many cases, e.g. "\Librecores\ProjectRepoBundle\Entity\ClassificationHierarchy" can be shortened into "ClassificationHierarchy".
    • Look through the phpdoc comments and make sure they contain useful content for functions and member variables you created. For example, the one in ProjectClassification is almost empty, the one for ProjectClassification::project is only newlines.
  2. ClassificationHierarchy:
    • you don't need updated and created dates here.
  3. ProjectClassification
    • Add an index on the classification field, as it is the main search target.
    • Add a phpdoc description that the classification field actually contains a string classification for easier searching, how the classification hierarchy levels are separated, etc.
  4. Migrations/initial data/development data
    • Migrations are there to cope with evolving database schemas. The list of available of categories doesn't really fit into this picture. In fact they're more a "configuration file" which happens to be stored in the database. This "configuration file" is stored in the librecores-web git repository and used to update the representation in the database.
    • Therefore let's use a method which actually accounts for this: store the initial categories in some form of separate file, and provide a way to insert them into the DB.
    • I see two main options to store the list of categories:
      a. specify the categories as PHP code (as you have done). In this case make sure to save this data into a separate PHP file.
      b. specify the categories as SQL statements
    • Then you need to insert this data into the DB. I'd create a fixture that reads this data and inserts it into the DB. On production servers we can run doctrine:fixtures:load with this specific fixture to update the CategoriesHierarchy in the DB with what we have specified in the "configuration file".
    • To avoid the circular dependency errors when inserting this data into the DB you can temporarily disable foreign key checks (or use a transaction, which should also result in the foreign keys being checked only once for the full transaction).
*/
public function up(Schema $schema)
{
// this up() migration is auto-generated, please modify it to your needs

This comment has been minimized.

Copy link
@agathver

agathver May 28, 2018

Collaborator

Are you sure you are creating the table before insert? Does the migration is in the other PR create the table?

@agathver

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2018

@imphil

Then you need to insert this data into the DB. I'd create a fixture that reads this data and inserts it into the DB. On production servers we can run doctrine:fixtures:load with this specific fixture to update the CategoriesHierarchy in the DB with what we have specified in the "configuration file".

I will suggest not to use fixtures for this, fixtures are meant to bw used for loading test data. By default, the fixtures delete the entire database. If we are a little careless while executing them, it will delete the production database.

I would consider this as an acceptable practice to insert the categories through migration. Other frameworks like Django, also encourage this.

Alternatively, if we want to be able to update the categories from a yaml file, independently of the deployment, I would propose to create a simple Symfony command to do the same. In Symfony commands, we have full access to Doctrine ORM and is relatively safe.

@imphil

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

@agathver

I agree with you about the danger of using fixtures for inserting data. The idea was to have the correct command wrapped in an Ansible deployment role, so it would be less likely to shoot yourself. However your YAML-approach sounds good and is probably even easier to implement (and to read) than storing the data in a PHP file with the array-structure Sandip has used. Could we possibly even use the YAML-based configuration infrastructure that Symfony already provides?

Why don't I like using migrations? The hierarchy of categories is configuration. In my experience, configuration is handled best as state, not as modification. As a admin, you want to be sure that your system has a given configuration, not hope that the migration steps have led to the right result. The same goes for recovering from a corrupt intermediate state.

Please note that I'm not blocking this PR based on the migrations vs. state issue. Once we have a working solution we can go ahead and merge this, a follow-up issue is always possible.

@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2018

@imphil I am documented the ProjectClassification and ClassificationHierarchy Entities.

fatal: [librecores]: FAILED! => {"changed": true, "cmd": ["php", "bin/console", "doctrine:migrations:migrate", "-n", "--no-ansi"], "delta": "0:00:02.255971", "end": "2018-05-28 14:51:01.150210", "msg": "non-zero return code", "rc": 1, "start": "2018-05-28 14:50:58.894239", "stderr": "14:51:01 ERROR [console] Error thrown while running command "doctrine:migrations:migrate -n --no-ansi". Message: "An exception occurred while executing 'INSERT INTO ClassificationHierarchy(parent_id,name,created_at,updated_at) VALUES (?,?,?,?)' with params [null, "License", "2018-05-28 14:51:01", "2018-05-28 14:51:01"]:\n\nSQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.ClassificationHierarchy' doesn't exist"

This bug came cause I haven't written the creation of ClassifciationHierarchy table in migration. I will do that after we decide a particular way to insert the data into ClassificationHierarchy table. I think I can configure the YAML structure from which I will insert the data into the schemas.

sandipbhuyan added some commits May 29, 2018

Add PHPDoc and write query to create ClassificationHierarchy schema i…
…n the migration

Removed fully classified name in Project Entities and Documentaion added for implemented feature.
@imphil

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

There's still code missing to create the ProjectClassification database table. How did you create the migration? The auto-generated migration code should have everything set up. Before you call this ready the next time please take do a vagrant destroy && vagrant up.

fatal: [librecores]: FAILED! => {"changed": true, "cmd": ["php", "bin/console", "doctrine:fixtures:load", "-n"], "delta": "0:00:00.541436", "end": "2018-05-29 20:45:00.774845", "msg": "non-zero return code", "rc": 1, "start": "2018-05-29 20:45:00.233409", "stderr": "20:45:00 ERROR     [console] Error thrown while running command \"doctrine:fixtures:load -n\". Message: \"An exception occurred while executing 'DELETE FROM ProjectClassification':\n\nSQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.ProjectClassification' doesn't exist\" [\"error\" => Doctrine\\DBAL\\Exception\\TableNotFoundException { …},\"command\" => \"doctrine:fixtures:load -n\",\"message\" => \"\"\"  An exception occurred while executing 'DELETE FROM ProjectClassification':\\n  \\n  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.ProjectClassification' doesn't exist  \"\"\"] []\n\nIn AbstractMySQLDriver.php line 57:\n                                                                               \n  An exception occurred while executing 'DELETE FROM ProjectClassification':   \n                                                                               \n  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.Proje  \n  ctClassification' doesn't exist                                              \n                                                                               \n\nIn PDOConnection.php line 59:\n                                                                               \n  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.Proje  \n  ctClassification' doesn't exist                                              \n                                                                               \n\nIn PDOConnection.php line 57:\n                                                                               \n  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.Proje  \n  ctClassification' doesn't exist                                              \n                                                                               \n\ndoctrine:fixtures:load [--fixtures [FIXTURES]] [--append] [--em EM] [--shard SHARD] [--purge-with-truncate] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] <command>", "stderr_lines": ["20:45:00 ERROR     [console] Error thrown while running command \"doctrine:fixtures:load -n\". Message: \"An exception occurred while executing 'DELETE FROM ProjectClassification':", "", "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.ProjectClassification' doesn't exist\" [\"error\" => Doctrine\\DBAL\\Exception\\TableNotFoundException { …},\"command\" => \"doctrine:fixtures:load -n\",\"message\" => \"\"\"  An exception occurred while executing 'DELETE FROM ProjectClassification':\\n  \\n  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.ProjectClassification' doesn't exist  \"\"\"] []", "", "In AbstractMySQLDriver.php line 57:", "                                                                               ", "  An exception occurred while executing 'DELETE FROM ProjectClassification':   ", "                                                                               ", "  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.Proje  ", "  ctClassification' doesn't exist                                              ", "                                                                               ", "", "In PDOConnection.php line 59:", "                                                                               ", "  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.Proje  ", "  ctClassification' doesn't exist                                              ", "                                                                               ", "", "In PDOConnection.php line 57:", "                                                                               ", "  SQLSTATE[42S02]: Base table or view not found: 1146 Table 'librecores.Proje  ", "  ctClassification' doesn't exist                                              ", "                                                                               ", "", "doctrine:fixtures:load [--fixtures [FIXTURES]] [--append] [--em EM] [--shard SHARD] [--purge-with-truncate] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] <command>"], "stdout": "  > purging database", "stdout_lines": ["  > purging database"]}
@imphil
Copy link
Contributor

left a comment

@sandipbhuyan Unfortunately we're still not done with this one. You can increase speed by reducing the number of roundtrips if you take greatest care to all details. Do a full testing before you submit a PR for review. Read through the diff to spot any spelling or formatting mistakes. Etc.

* This class holds the default data
*
*/

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

no empty line here

* @var array classifiers
*
*/
const classifier = [

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

Now the classifiers are in the DataConfig table and in here? Why the duplication?

*
*/
const classifier = [
[NULL, "License"],

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

This array would be way easier to read if you added the index explicitly to it, e.g. 0 => ...

*
*/
class DataConfig

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

This class is both named way too generic and in a place where typically no classes are in Symfony. Simply leave this data inside the migration, having it anywhere else doesn't help as a new migration needs to be created anyways if this data changes.

/**
* A classification hierarchy for the projects
*
* It contains classification categories that can be use to classify the IP Cores

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

s/It/This class/
s/the IP Cores/a project/

private $parent;
/**
* It holds the category name of the project classification hierarchy

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

The typical way to write such a comment would be simply "Category name"

@@ -71,6 +72,16 @@ class Project
*/
private $parentOrganization;
// Associations
/**
* @var ArrayCollection

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

missing doxygen comment.
"Classifications assigned to this project"

/**
* It contains the classification string which has been classified by
* facted classification. Classifications are categorization of a project
* and two consecutive categories will be separated by '::'

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor
  • Describe state, not plans: It's not "will be", it's "are"
  • The first sentence is circular. It's not a string that has been classified by a classification. It's the project which is classified by assigning a classifier to it.
  • Keep the doxygen comment format in mind: first line is summary, then a newline, then the long text (if any)
/**
* It contains the project id and related to Project Entity in
* One Project can have Many ProjectClassification
*

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

same as above.

private $project;
/**
* It conatins the creation date of the classification

This comment has been minimized.

Copy link
@imphil

imphil May 29, 2018

Contributor

typo, and remove the "It contains the". Simply "Creation date/time" is sufficient.

@agathver agathver changed the title Create database entities and update the databases Create database entities and update the database May 30, 2018

Recreate the migration file and Specify PHPDoc for the implemented fu…
…nctions

A new migration file is being recreated and insert query for ClassificationHierarchy schema is written in it. PHPDoc comments are modified in implemented functions
@sandipbhuyan

This comment has been minimized.

Copy link
Collaborator Author

commented May 30, 2018

@imphil I have tested the project. I have updated the PHPDoc comments on the implemented features.
Have a look on it when you are free

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:database-extension branch from 22e4a41 to 19b5193 Jun 4, 2018

/**
*Populating classificatioinHierarchy object for development environment
*
* These are the test data for classification hierarchy. For the complete

This comment has been minimized.

Copy link
@agathver

agathver Jun 4, 2018

Collaborator

👍

@agathver
Copy link
Collaborator

left a comment

LGTM, save for the small code style changes.

$manager->persist($verilog);
//ClassificationHierarchy Programming Language::Verilog::Verilog 95
$verilog_95 = new ClassificationHierarchy();

This comment has been minimized.

Copy link
@agathver

agathver Jun 4, 2018

Collaborator

No snake_case.

verilog95

@agathver
Copy link
Collaborator

left a comment

LGTM

@sandipbhuyan sandipbhuyan force-pushed the sandipbhuyan:database-extension branch from 4d24c7c to 18fcca4 Jun 11, 2018

@imphil

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

I've now pushed these changes to master as 65adca5 with some modifications applied.

  • The classification hierarchy is stored in a YAML file as discussed.
  • An index for the ProjectClassification.classification column is added.
  • A ProjectClassification.createdBy column is added.
  • Some minor code and naming style adjustments.

Please look through those changes and take them as the base for your future work.

@imphil imphil closed this Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.