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

Bugfix report different behavior based on path #262

Conversation

chocoelho
Copy link
Contributor

Resolves #257.

@carlio this will really need your input on it. To summarize, that was a point where prospector would give different results, this should be fixed. Also, pep8linter was not using all found files depending on the path it was called.

@chocoelho chocoelho requested a review from carlio July 23, 2018 13:36
@coveralls
Copy link

coveralls commented Jul 23, 2018

Pull Request Test Coverage Report for Build 617

  • 26 of 28 (92.86%) changed or added relevant lines in 4 files are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.1%) to 76.402%

Changes Missing Coverage Covered Lines Changed/Added Lines %
prospector/tools/pep8/init.py 17 19 89.47%
Files with Coverage Reduction New Missed Lines %
prospector/tools/pep8/init.py 1 90.72%
prospector/message.py 1 72.88%
prospector/tools/pylint/collector.py 9 82.76%
Totals Coverage Status
Change from base Build 615: 0.1%
Covered Lines: 1512
Relevant Lines: 1979

💛 - Coveralls

@carlio
Copy link
Member

carlio commented Jul 23, 2018

@chocoelho are you using an automatic formatter? Most of the changes are cosmetic which makes it hard to figure out what actual functionality has changed.

I'm all for having a conistent coding style - I'm a big fan of black and there's a pre-commit hook to add to .pre-commit.yml if we want to. I just think that either we don't use one or we use the same one to avoid too much noise in PRs.

@chocoelho
Copy link
Contributor Author

chocoelho commented Jul 23, 2018 via email

@@ -187,7 +188,7 @@ def _find_paths(ignore, curpath, rootpath):
return files, modules, packages, directories


def find_python(ignores, paths, explicit_file_mode, workdir=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check here

@@ -198,5 +199,5 @@ def find_python(ignores, paths, explicit_file_mode, workdir=None):
return SingleFiles(paths, workdir or os.getcwd())
else:
assert len(paths) == 1
files, modules, directories, packages = _find_paths(ignores, paths[0], paths[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check here

@carlio
Copy link
Member

carlio commented Jul 23, 2018

I think it'd be best to avoid it because you can't guarantee every person is using the same one and PRs would get very difficult to keep track of. So if you could turn it off then create a PR with just the functional changes that'd be great - otherwise any PR from anyone would conflict in annoying ways!

external_config = conf_path
break

# create a list of packages, but don't include packages which are
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check here

@chocoelho
Copy link
Contributor Author

I'll rebase it with what indeed changed. I touched a some other files while debugging some parts of the code.

Signed-off-by: Carlos Coêlho <carlospecter@gmail.com>
Signed-off-by: Carlos Coêlho <carlospecter@gmail.com>
Signed-off-by: Carlos Coêlho <carlospecter@gmail.com>
Signed-off-by: Carlos Coêlho <carlospecter@gmail.com>
@chocoelho chocoelho force-pushed the bugfix-report-different-behavior-based-on-path branch from e0394fe to 44116a0 Compare July 24, 2018 22:50
@chocoelho
Copy link
Contributor Author

@carlio updated the PR the needed changes

Signed-off-by: Carlos Coêlho <carlospecter@gmail.com>
@chocoelho chocoelho merged commit 4dd55e4 into landscapeio:develop Jul 31, 2018
@chocoelho chocoelho deleted the bugfix-report-different-behavior-based-on-path branch July 31, 2018 02: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.

None yet

3 participants