Bug29221 draft #742
Bug29221 draft #742
Conversation
No longer messes up with MOCK_IMPL. Also update the tests to show that. We are still being innacurate on the line count in some cases, but that doesnt matter so much.
- Introduce 'make check-best-practices'. - Fix up Tor topdir etc to work with the way 'make check-local' gets called. - Make practracker less likely to print useless stuff.
Pull Request Test Coverage Report for Build 4169
|
This was causing issues because the exceptions file is written using Posix
paths, whereas practracker in Windows was trying to match Windows paths ("\"
instead of "/").
scripts/maint/practracker/metrics.py
Outdated
| func_start = lineno | ||
| in_function = True | ||
| else: | ||
| # Fund the end of a function |
Oops, this is my typo: this function should say "find", not "fund"
Fixed in ac05cc7.
| """Get number of #include statements in the file""" | ||
| include_count = 0 | ||
| for line in f: | ||
| if re.match(r' *# *include', line): |
Maybe this should be \s* instead of *, since tabs can also appear in this context. On the other hand, Tor doesn't allow tabs, so maybe we don't need to care.
Included this improvement in the post-merge ticket #29746, since it's a good-to-have but doesn't actually influence results atm. Ideally we would have tests for this change.
| super(FunctionSizeProblem, self).__init__("function-size", problem_location, metric_value) | ||
|
|
||
| def get_old_problem_from_exception_str(exception_str): | ||
| try: |
I'd suggest that we explicitly allow comments and empty lines in this file, and that we treat other unrecognized lines as errors.
Included this improvement in the post-merge ticket #29746, since it's a good-to-have but doesn't actually influence results atm. Ideally we would have tests for such change.
scripts/maint/practracker/problem.py
Outdated
| elif problem_type == "function-size": | ||
| return FunctionSizeProblem(problem_location, metric_value) | ||
| else: | ||
| print "Unknown exception line %s" % exception_str |
Using stderr might make sense here; also, this won't work with python3. (Maybe you fix it later in the branch)
Yep, that line was fixed later in the branch, but I used stderr for another err message in ac05cc7.
scripts/maint/practracker/metrics.py
Outdated
| """ | ||
| Return iterator which iterates over functions and returns (function name, function lines) | ||
| """ | ||
|
|
||
| # XXX Buggy! Doesn't work with MOCK_IMPL and ENABLE_GCC_WARNINGS | ||
| # Skip lines with these terms since they confuse our regexp | ||
| REGEXP_CONFUSE_TERMS = ["MOCK_IMPL", "ENABLE_GCC_WARNINGS", "ENABLE_GCC_WARNING", "DUMMY_TYPECHECK_INSTANCE", |
I'd suggest that you use a set here rather than an array.
Fixed in ac05cc7.
| in_function = False | ||
| for lineno, line in enumerate(f): | ||
| if any(x in line for x in REGEXP_CONFUSE_TERMS): |
Instead of checking for the presence of the strings here, I'd suggest checking after we find the start of a function, to see if the function name is in the list of confusing terms.
Added to post-merge ticket #29746. Ideally we would have tests for this change to make sure that results are not impacted.
I think this one is actually important for correctness; I'll try a quick fix. See e968b88 in my bug29221_more
|
|
||
| def consider_function_size(fname, f): | ||
| for name, lines in metrics.function_lines(f): | ||
| """Consider the function sizes for 'f' and return True if a new issue was found""" | ||
| found_new_issues = False |
Suggestion, possibly for a later ticket: use a counter here instead of a boolean. That way you can print the number of new issues on exit.
Added to post-merge ticket #29746.
scripts/maint/practracker/util.py
Outdated
| @@ -14,7 +18,7 @@ def get_tor_c_files(tor_topdir, exclude_dirs): | |||
|
|
|||
| # Exclude the excluded paths | |||
| full_path = os.path.join(root,filename) | |||
| if any(exclude_dir in full_path for exclude_dir in exclude_dirs): | |||
| if any(exclude_dir in full_path for exclude_dir in EXCLUDE_SOURCE_DIRS): | |||
I don't like looking for the excluded path everywhere in the full_path string; it should only be excluded when it is right under TOP_SRCDIR. In other words, if tor is in ~/tor/, then ~/tor/src/trunnel/ should be excluded, but "~/src/tor/src/lib/handle_trunnel/src/trunnel` shouldn't be excluded
I tried to do this, but I think it should be done properly with tests to make sure of it's correctness.
Added to post-merge ticket #29746.
| if (found_new_issues): | ||
| print("practracker FAILED") | ||
| new_issues_str = "practracker FAILED as indicated by the problem lines above. Please use the exceptions file ({}) to find any previous state of these problems. If you are unable to fix the underlying best-practices issue right now then you need to either update the relevant exception line or add a new one.".format(exceptions_file) |
With your permission, I'll tweak this string a little post-merge. The way it is written now, it suggests that the preferred solution to a bad practice is to add an exception, rather than to fix the problem. I think there should be a new section in doc/HACKING about this, with suggestions for how to fix each kind of problem.
Yes, no problem with tweaking that string.
|
eventually merged as #787 |
No description provided.
The text was updated successfully, but these errors were encountered: