Fix: Terminal cursor disappears after aborting scan with Ctrl+ #164
Conversation
…text` for cleaner implementation
WalkthroughRefactors spinner/progress handling to context managers with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant ScannerHFH
participant SpinnerCtx as "Spinner Context\n(Spinner / nullcontext)"
participant Worker as "Worker Thread"
participant GRPC as "gRPC Service"
User->>CLI: request folder-hash scan
CLI->>ScannerHFH: scan_folder(hfh_request)
ScannerHFH->>SpinnerCtx: enter with Spinner or nullcontext
ScannerHFH->>Worker: start _execute_grpc_scan(hfh_request)
note right of Worker `#f8f9fb`: background thread performs gRPC call
Worker->>GRPC: perform scan call
GRPC-->>Worker: return results
Worker-->>ScannerHFH: assign scan_results
ScannerHFH->>Worker: join (wait)
SpinnerCtx--)ScannerHFH: exit
ScannerHFH-->>CLI: return scan_results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas to focus on:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/scanoss/filecount.py(2 hunks)src/scanoss/scanner.py(5 hunks)src/scanoss/scanners/folder_hasher.py(2 hunks)src/scanoss/scanners/scanner_hfh.py(3 hunks)src/scanoss/threadedscanning.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/scanoss/filecount.py (2)
src/scanoss/cli.py (1)
file_count(1297-1328)src/scanoss/scanossbase.py (1)
print_trace(65-70)
src/scanoss/scanners/folder_hasher.py (2)
src/scanoss/scanossbase.py (1)
print_debug(58-63)src/scanoss/utils/crc64.py (2)
CRC64(29-96)get_hash_buff(82-96)
src/scanoss/scanner.py (5)
src/scanoss/file_filters.py (2)
get_filtered_files_from_folder(311-352)get_filtered_files_from_files(354-404)src/scanoss/threadedscanning.py (3)
stop_scanning(154-158)queue_add(141-149)run(168-191)src/scanoss/winnowing.py (1)
wfp_for_file(269-282)src/scanoss/scanners/scanner_hfh.py (1)
scan(127-153)src/scanoss/scanossapi.py (1)
scan(134-244)
src/scanoss/scanners/scanner_hfh.py (2)
src/scanoss/scanossgrpc.py (1)
folder_hash_scan(429-447)src/scanoss/scanossbase.py (1)
print_stderr(45-49)
🪛 GitHub Actions: Lint
src/scanoss/filecount.py
[error] 100-100: PLR0912 Too many branches (15 > 12)
[error] 100-100: PLR0915 Too many statements (54 > 50)
[error] 109-109: F541 [*] f-string without any placeholders. Remove extraneous f prefix.
[error] 152-152: PLC0206 Extracting value from dictionary without calling .items()
src/scanoss/scanners/folder_hasher.py
[error] 3-3: F401 'contextlib.nullcontext' imported but unused
src/scanoss/scanner.py
[error] 397-397: E501 Line too long (121 > 120)
[error] 416-416: E501 Line too long (124 > 120)
[error] 802-802: E501 Line too long (122 > 120)
src/scanoss/scanners/scanner_hfh.py
[error] 28-28: F401 'contextlib.nullcontext' imported but unused
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
eeisegn
left a comment
eeisegn
left a comment
There was a problem hiding this comment.
Need to address linter issues also please
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/scanoss/filecount.py (3)
29-29: Context-managed spinner usage looks correct and robustUsing
nullcontext()whenquietor not TTY, combined withwith spinner_ctx as spinner:and theif spinner:guard, is a clean way to ensure the spinner is only active in interactive terminals and that cleanup happens even onKeyboardInterrupt. This should address the cursor-not-restored issue without changing counting semantics.If you find this pattern repeating elsewhere, consider extracting a small helper (e.g.
self._spinner_ctx(label: str)) to centralize thequiet/TTY logic and avoid duplication across modules.Also applies to: 114-147
100-147: Refactorcount_filesto satisfy PLR0912/PLR0915 and reduce complexityThe linter errors (
PLR0912 Too many branches,PLR0915 Too many statements) are triggered by the size and branching insidecount_files. The logic is fine, but it’s a bit dense.You can simplify by extracting the inner walk/count logic and the summary/CSV writing into helpers so
count_filesstays small and orchestration-only.One possible refactor:
def count_files(self, scan_dir: str) -> bool: @@ - success = True + success = True if not scan_dir: raise Exception('ERROR: Please specify a folder to scan') if not os.path.exists(scan_dir) or not os.path.isdir(scan_dir): raise Exception(f'ERROR: Specified folder does not exist or is not a folder: {scan_dir}') self.print_msg(f'Searching {scan_dir} for files to count...') spinner_ctx = Spinner('Searching ') if (not self.quiet and self.isatty) else nullcontext() with spinner_ctx as spinner: - file_types = {} - file_count = 0 - file_size = 0 - for root, dirs, files in os.walk(scan_dir): - self.print_trace(f'U Root: {root}, Dirs: {dirs}, Files {files}') - dirs[:] = self.__filter_dirs(dirs) # Strip out unwanted directories - filtered_files = self.__filter_files(files) # Strip out unwanted files - self.print_trace(f'F Root: {root}, Dirs: {dirs}, Files {filtered_files}') - for file in filtered_files: # Cycle through each filtered file - path = os.path.join(root, file) - f_size = 0 - try: - f_size = os.stat(path).st_size - except Exception as e: - self.print_trace(f'Ignoring missing symlink file: {file} ({e})') # broken symlink - if f_size > 0: # Ignore broken links and empty files - file_count = file_count + 1 - file_size = file_size + f_size - f_suffix = pathlib.Path(file).suffix - if not f_suffix or f_suffix == '': - f_suffix = 'no_suffix' - self.print_trace(f'Counting {path} ({f_suffix} - {f_size})..') - fc = file_types.get(f_suffix) - if not fc: - fc = [1, f_size] - else: - fc[0] = fc[0] + 1 - fc[1] = fc[1] + f_size - file_types[f_suffix] = fc - if spinner: - spinner.next() - # End for loop - self.print_stderr(f'Found {file_count:,.0f} files with a total size of {file_size / (1 << 20):,.2f} MB.') - if file_types: - csv_dict = [] - for k in file_types: - d = file_types[k] - csv_dict.append({'extension': k, 'count': d[0], 'size(MB)': f'{d[1] / (1 << 20):,.2f}'}) - fields = ['extension', 'count', 'size(MB)'] - file = sys.stdout - if self.scan_output: - file = open(self.scan_output, 'w') - writer = csv.DictWriter(file, fieldnames=fields) - writer.writeheader() # writing headers (field names) - writer.writerows(csv_dict) # writing data rows - if self.scan_output: - file.close() - else: - FileCount.print_stderr(f'Warning: No files found to count in folder: {scan_dir}') + file_types, file_count, file_size = self._walk_and_count(scan_dir, spinner) + + self._report_counts(scan_dir, file_types, file_count, file_size) return successAnd then add (outside this method):
def _walk_and_count(self, scan_dir: str, spinner): file_types: dict[str, list[int | float]] = {} file_count = 0 file_size = 0 for root, dirs, files in os.walk(scan_dir): self.print_trace(f'U Root: {root}, Dirs: {dirs}, Files {files}') dirs[:] = self.__filter_dirs(dirs) filtered_files = self.__filter_files(files) self.print_trace(f'F Root: {root}, Dirs: {dirs}, Files {filtered_files}') for file in filtered_files: path = os.path.join(root, file) try: f_size = os.stat(path).st_size except Exception as e: # broken symlink self.print_trace(f'Ignoring missing symlink file: {file} ({e})') continue if f_size <= 0: continue file_count += 1 file_size += f_size f_suffix = pathlib.Path(file).suffix or 'no_suffix' self.print_trace(f'Counting {path} ({f_suffix} - {f_size})..') fc = file_types.get(f_suffix) if not fc: fc = [1, f_size] else: fc[0] += 1 fc[1] += f_size file_types[f_suffix] = fc if spinner: spinner.next() return file_types, file_count, file_sizedef _report_counts(self, scan_dir: str, file_types, file_count: int, file_size: int) -> None: self.print_stderr(f'Found {file_count:,.0f} files with a total size of {file_size / (1 << 20):,.2f} MB.') if file_types: csv_dict = [ {'extension': ext, 'count': d[0], 'size(MB)': f'{d[1] / (1 << 20):,.2f}'} for ext, d in file_types.items() ] fields = ['extension', 'count', 'size(MB)'] file = sys.stdout if self.scan_output: file = open(self.scan_output, 'w') try: writer = csv.DictWriter(file, fieldnames=fields) writer.writeheader() writer.writerows(csv_dict) finally: if self.scan_output: file.close() else: FileCount.print_stderr(f'Warning: No files found to count in folder: {scan_dir}')This should bring both branch and statement counts per function back under the linter thresholds while keeping behavior identical.
152-154: Fix PLC0206 by iterating overfile_types.items()The linter warning
PLC0206 Extracting value from dictionary without calling .items()is about thefor k in file_types:+d = file_types[k]pattern. You can simplify and satisfy the linter by iterating overitems()directly.Suggested diff:
- csv_dict = [] - for k in file_types: - d = file_types[k] - csv_dict.append({'extension': k, 'count': d[0], 'size(MB)': f'{d[1] / (1 << 20):,.2f}'}) + csv_dict = [] + for extension, data in file_types.items(): + csv_dict.append( + { + 'extension': extension, + 'count': data[0], + 'size(MB)': f'{data[1] / (1 << 20):,.2f}', + } + )This keeps behavior the same and clears the PLC0206 violation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scanoss/filecount.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/scanoss/filecount.py (2)
src/scanoss/scanossbase.py (2)
print_msg(51-56)print_trace(65-70)src/scanoss/cli.py (1)
file_count(1297-1328)
🪛 GitHub Actions: Lint
src/scanoss/filecount.py
[error] 100-100: PLR0912 Too many branches (15 > 12)
[error] 100-100: PLR0915 Too many statements (54 > 50)
[error] 152-152: PLC0206 Extracting value from dictionary without calling .items()
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Fixes: SP-3669
Problem
When users abort scanoss-py commands with Ctrl+C, the terminal cursor remains hidden, making the terminal appear broken until reset.
Root Cause:
The progress library (Spinner/Bar) hides the cursor on initialization but only restores it when finish() is called. On KeyboardInterrupt, finish() was never executed, leaving the cursor hidden.
Testing
Tested cursor restoration on Ctrl+C for:
Summary by CodeRabbit
Refactor
Bug Fixes