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

Merge functional #20

Merged
merged 38 commits into from
Aug 6, 2018
Merged

Merge functional #20

merged 38 commits into from
Aug 6, 2018

Conversation

sharanry
Copy link
Contributor

@sharanry sharanry commented Aug 6, 2018

No description provided.

Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

Do you know what needs to be done to fix the build? Looks fine to me otherwise.

@@ -0,0 +1,2 @@
[pycodestyle]
max-line-length = 120
Copy link
Member

Choose a reason for hiding this comment

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

this file should be removed (pylint handles this check)

Copy link
Contributor Author

@sharanry sharanry Aug 6, 2018

Choose a reason for hiding this comment

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

i think its overriding the default of 79 characters

.pylintrc Outdated
@@ -323,7 +323,7 @@ ignore-on-opaque-inference=yes
# List of class names for which member attributes should not be checked (useful
# for classes with dynamically set attributes). This supports the use of
# qualified names.
ignored-classes=optparse.Values,thread._local,_thread._local
ignored-classes=optparse.Values,thread._local,_thread._local, Context
Copy link
Member

Choose a reason for hiding this comment

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

is Context still needed to be ignored?

.pylintrc Outdated
@@ -54,7 +54,7 @@ confidence=
# --enable=similarities". If you want to run only the classes checker, but have
# no Warning level messages displayed, use"--disable=all --enable=classes
# --disable=W"
disable=missing-docstring
disable=missing-docstring, C0103
Copy link
Member

Choose a reason for hiding this comment

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

should use full name for C0103 (i think invalid-constant-name)

- pytest -xv --cov=pymc4/ test/
- python -m pylint pymc4/
- python -m pytest -xv --cov=pymc4/ tests/
- pycodestyle --show-source --show-pep8 pymc4/ tests/
Copy link
Member

Choose a reason for hiding this comment

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

ah, hadn't noticed pycodestyle is still being run. is there a good reason for that and pylint? in my experience it is faster, but less comprehensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferrine had added it. Any reasons?

Copy link
Member

Choose a reason for hiding this comment

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

I believe pycodestyle is better for style checks and linter for spotting typos

@coveralls
Copy link

Pull Request Test Coverage Report for Build 64

  • 197 of 233 (84.55%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+84.6%) to 84.615%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pymc4/distributions/base.py 0 6 0.0%
pymc4/util/interceptors.py 84 94 89.36%
pymc4/util/graph.py 4 24 16.67%
Totals Coverage Status
Change from base Build 61: 84.6%
Covered Lines: 198
Relevant Lines: 234

💛 - Coveralls

@ColCarroll ColCarroll merged commit ac0d01e into master Aug 6, 2018
@ColCarroll
Copy link
Member

Thanks, @sharanry, @ferrine, and @jsafyan!

@canyon289 canyon289 deleted the functional branch January 20, 2019 15:02
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.

6 participants