Properties transformer - Githubissues

Cerfoglg commented 8 years ago

[x] General Comment
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg map the related issues, please. We are gonna merge this, when it is integrated with tests and in the final state of refactoring.
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme Ah, the issue regarding properties was only opened for the collector of the properties. See the pull I made for the collectors regarding it
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Moreover make sure that you save only once the same host_properties. There should be a single value that let you identify if the host is the same, e.g., ID
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> - Move code away from the main
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> @Cerfoglg ping me when it's ready to the final review
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme Should be ready now
[x] File: data-transformers/transformers/propertiesTransformer.py:L192-264
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> I guess this is a common function to multiple transformers. Are you going to refactor it in this pull request or in a new one? If it's a new one, open an issue about this and point to this comment
[x] File: data-transformers/transformers/propertiesTransformer.py:L192-264
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Why do you need to access to the first object in the returned array? What is the returned array?
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> :+1:
<img border=0 src='https://avatars.githubusercontent.com/u/8819678?v=3' height=16 width=16'> @VincenzoFerme It's a list containing all the lines in the file. But these files only have one line, so I just take that
[x] File: data-transformers/transformers/propertiesTransformer.py:L1-270
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> Answer to the following, please: https://github.com/benchflow/data-transformers/pull/50/files/4f9492676ef254ee77e9ad986fea2117cd1125e3..363462c7879c685413dead534ec2e70bbc1644f4#r63614569
[x] File: data-transformers/transformers/propertiesTransformer.py:L1-270
<img border=0 src='https://avatars.githubusercontent.com/u/7163641?v=3' height=16 width=16'> This should go in a commons library of data-transformers. I marked it in #5

VincenzoFerme commented 8 years ago

@Cerfoglg map the related issues, please. We are gonna merge this, when it is integrated with tests and in the final state of refactoring.

Cerfoglg commented 8 years ago

@VincenzoFerme Ah, the issue regarding properties was only opened for the collector of the properties. See the pull I made for the collectors regarding it

VincenzoFerme commented 8 years ago

Moreover make sure that you save only once the same host_properties. There should be a single value that let you identify if the host is the same, e.g., ID

Cerfoglg commented 8 years ago

Move code away from the main

VincenzoFerme commented 8 years ago

@Cerfoglg ping me when it's ready to the final review

Cerfoglg commented 8 years ago

@VincenzoFerme Should be ready now

VincenzoFerme commented 8 years ago

:+1:

VincenzoFerme commented 8 years ago

:shipit:

benchflow / data-transformers

Properties transformer #50