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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Python 3 #3029

Merged
merged 4 commits into from
Jan 28, 2019
Merged

Support Python 3 #3029

merged 4 commits into from
Jan 28, 2019

Conversation

gmmeyer
Copy link
Contributor

@gmmeyer gmmeyer commented Jan 24, 2019

What does this PR do?

We need to support python 3 in all things. This check was a rough one and I definitely need someone to look it over pretty thoroughly to ensure I did everything right.

Motivation

馃悕 3锔忊儯 馃殏

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If PR adds a configuration option, it has been added to the configuration file.

Additional Notes

Anything else we should know when reviewing?

@codecov-io
Copy link

codecov-io commented Jan 24, 2019

Codecov Report

Merging #3029 into master will increase coverage by 10.98%.
The diff coverage is 85.83%.

@@             Coverage Diff             @@
##           master    #3029       +/-   ##
===========================================
+ Coverage   84.15%   95.13%   +10.98%     
===========================================
  Files         673       19      -654     
  Lines       36595     5205    -31390     
  Branches     4336      197     -4139     
===========================================
- Hits        30798     4952    -25846     
+ Misses       4548      181     -4367     
+ Partials     1249       72     -1177

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Nice!

-e../datadog_checks_base[deps]
-rrequirements-dev.txt
unit: -e../datadog_checks_base[deps]
unit: -rrequirements-dev.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

We need these no matter what so let's remove the specifiers

commands =
pip install -r requirements.in
pytest -v
unit: pip install -r requirements.in
Copy link
Contributor

Choose a reason for hiding this comment

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

This too

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

Looks good to me, left a couple minor nits. Also 馃挏 the test refactors, nice job.

import aci_metrics
from six import iteritems

from . import aci_metrics
from . import helpers
from . import exceptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could these be imported from the same line?

import aci_metrics
from six import iteritems

from . import aci_metrics
from . import helpers
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be one line too?

'DataDog',
],
"tags": ["project:cisco_aci"],
}


@pytest.fixture
def aggregator():
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this too as its a global fixture now.

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

Looks good to me once CI passes, thanks!

@gmmeyer gmmeyer merged commit 4ca83f0 into master Jan 28, 2019
@gmmeyer gmmeyer deleted the greg/cisco branch January 28, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants