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

Updated Readme with sample configurations , Added Tests and also fixed some more issues. #29

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

manishgcjain
Copy link

Hi Martin,
As per your suggestion I have updated documentation with sample configurations , Added Tests and also fixed some more issues.

Manish

@manishgcjain
Copy link
Author

Hi Martin,
Please help in approving this and ignore PR#28.

@manishgcjain
Copy link
Author

Hi Martin can you please help in approving the PR.

@optilude
Copy link
Contributor

Sorry for the delay. I think it’s fine to merge except:

  • it needs rebasing as I merged another PR! That’s the main one.
  • can we do a spell check on the readme?
  • can you run it through a python linter? Not critical really but there are many places where you’d expect a space before a comma or operator and it just looks a bit odd, which a linter would sort instantly.

@manishgcjain
Copy link
Author

Hi Martin,
I have not found any issue with linting and spell check on the readme also done without any errors.
Please help in approving the PR.
Manish

@optilude
Copy link
Contributor

optilude commented Oct 5, 2020

Hi,

The branch won't merge now because it needs rebaselining.

Will add some other comments inline in the PR.

Martin

Copy link
Contributor

@optilude optilude left a comment

Choose a reason for hiding this comment

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

I've added comments having reviewed most (but not all) of the code. My comments were starting to repeat themselves so I suggest you look to address those but also look for other examples of the same problems elsewhere.


# Defect density Segregated by Sprints

Following are the configuraitons for creating defect report charts by Sprint.
Copy link
Contributor

Choose a reason for hiding this comment

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

"configuraitons" is mis-spelled (here and in a few other places)

Copy link
Author

Choose a reason for hiding this comment

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

I will fix that.

Defects by priority chart: defects-by-priority.png
Defects by priority chart title: Defects by priority
Defects by type chart: defects-by-type.png
Defects by type chart title: Defects by type
Defects by environment chart: defects-by-environment.png
Defects by environment chart title: Defects by environment

#provide {} which would capture the team names provided in the configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this comment means

Copy link
Author

Choose a reason for hiding this comment

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

I will fix that with correct sample configuration.

# Get the fields
fixversion_field = self.settings['defects_fixversion_field']
fixversion_field_id = self.query_manager.field_name_to_id(fixversion_field) if fixversion_field else None
# Get the fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this comment duplicated?

Copy link
Author

Choose a reason for hiding this comment

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

I will fix that.

@@ -77,120 +90,198 @@ def run(self):

def write(self):
chart_data = self.get_result()
# fixversion_field_choice = self.settings['defects_fixversion_label']
# print(fixversion_field_choice)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't leave dead code commented out - remove if not needed

Copy link
Author

Choose a reason for hiding this comment

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

I will fix that.

if self.settings['defects_by_priority_chart']:
self.write_defects_by_priority_chart(chart_data, self.settings['defects_by_priority_chart'])

self.write_defects_by_priority_chart(chart_data, self.settings['defects_by_priority_chart'],x_label)
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a space after the comma (several more examples of this below)

Copy link
Author

Choose a reason for hiding this comment

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

I will fix that.


set_chart_style()
# set_chart_style()
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code

Copy link
Author

Choose a reason for hiding this comment

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

I will fix that.

window = self.settings['defects_window']
type_values = self.settings['defects_type_values']

if x_label !='Month':
Copy link
Contributor

Choose a reason for hiding this comment

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

Same point as above re comparator

Copy link
Author

Choose a reason for hiding this comment

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

config is initialised in config.py as below
'defects_fixversion_label': 'Month',
So if it is overridden then only the Sprint level charting code will be executed.


labels = [d.strftime("%b %y") for d in breakdown.index]
ax.set_xticklabels(labels, rotation=90, size='small')
if x_label =='Month':
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparator again

Copy link
Author

Choose a reason for hiding this comment

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

I will initialise it with a boolean in the start so that it can be used across.


set_chart_style()
# set_chart_style()
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code again

Copy link
Author

Choose a reason for hiding this comment

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

I will fix it.

breakdown = breakdown_by_month(chart_data, 'created', 'resolved', 'key', 'environment', environment_values)


if x_label !='Month':
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparator

Copy link
Author

Choose a reason for hiding this comment

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

I will initialize it with a boolean in the start so that it can be used across.

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