Remove matching lines from all files

Question

I have written a Python3 program that looks for matching strings and removes them from files.

import os

path = os.getcwd()


ignore_files = ["remove-lines.py", "to-remove"]

with open("to-remove", "r", newline="") as pattern_file:
    remove_patterns = [l for l in (line.strip() for line in pattern_file) if l]


for root, directories, files in os.walk(path, topdown=False):
    for name in files:
        out = []
        current_file = os.path.join(root, name)
        print(current_file)
        if any([ignore_file in current_file for ignore_file in ignore_files]):
            pass
        else:
            with open(current_file, "r") as in_file:
                in_file_lines = in_file.readlines()
            print("IN FILE:")
            print(in_file_lines)
            for in_file_line in in_file_lines:
                print(in_file_line)
                keep_var = True
                for remove_pattern in remove_patterns:
                    if remove_pattern in in_file_line:
                        keep_var = False
                        print("R: " + remove_pattern + " IS IN " + in_file_line)
                        break

                if keep_var:
                    print("OUTPUTTING")
                    print(in_file_line)
                    out.append(in_file_line)
            print("\n\n\nFINAL\n\n\n")
            print(out)
            with open(current_file, "w") as out_file:
                for line in out:
                    out_file.write(line)

The repo, with README, is on GitHub.

How could I improve this code? Are there ways I could make it more Pythonic? All feedback and criticisms welcome.

Answer 1

Picking the right tools

Your entire script can be written as the following one-liner

grep -RiIl 'search' | xargs sed -i's/.*\(here\|there\|why\).*//g'

Whenever your entire script can be expressed as a oneliner, it is a good indication that something needs to change. The onliner is intentially obtuse, but I would argue your code is equally obtuse. You just do not know it, because you wrote it ;) I combined the ideas from here

https://www.internalpointers.com/post/linux-find-and-replace-text-multiple-files

with .* to match anything zero or more times and then the list of banned words. Another solution from Overflow looks like this https://stackoverflow.com/questions/14443935/regex-search-replace-for-multiple-files-in-a-directory-using-python which is closer, but not quite where we want

Pythonic

What does pythonic mean? Does it mean "this code was written in Python"? No, see for instance Zen of Python.

Readability counts.

Take a step back. Breathe. Now look at your code. would you understand this code in a few hours, what about a few weeks, years? Your code is the first step to writing good code. Step 1, just make the freakin thing work.

A working code is like a book with a story but with no commas sections paragraphs commas or periods and it is just a big blob of text while the whole story is in there and every part makes sense to the author but it can be a big challenging for another person or your future self to read it dont you agree

Pythons chapters is files. Our table of contents is the docstring of each file. Then we put our code into paragraphs or functions, and explain the purpose of each paragraph. This is done by clear variable names, doctests and docstrings and typing hints.

Improvements

• Today we try to prefer to use the pathlib library to handle paths instead of os. (I know many still prefer walk), but learning to use the pathlib is never a bad idea.

• Explicit is better than implicit. You need to name "to-remove" better. 1) It needs a filetype to be specific. 2) You need to inform that this is a "user created file". user_words_to_remove.txt Notice how we used underscores as this is prefered by python. 3) This should really be a global constant, or even better prompted using sys.args or even better argparse (click is overkill)

• Learn to use early exits. This

   if any([ignore_file in current_file for ignore_file in ignore_files]):
        pass
    else:
        with open(current_file, "r") as in_file:
            in_file_lines = in_file.readlines()
        print("IN FILE:")
        print(in_file_lines)
  

  is better formated as

   if any([ignore_file in current_file for ignore_file in ignore_files]):
        continue
  
    with open(current_file, "r") as in_file:
        in_file_lines = in_file.readlines()
    print("IN FILE:")
    print(in_file_lines)
  

Do not print everywhere! Separate business logic from the UI. Have functions that have a sole purpose of printing, and one that has a sole purpose of fetching files. something like the below would be a start

    from pathlib import Path 

    def walk(path:Path, recursive:bool=False, depth:int=0) -> Path: 
        if depth > 0 and not recursive:
            return
        for p in Path(path).iterdir(): 
            if p.is_dir(): 
                yield from walk(p, depth=depth+1)
                continue
            yield p.resolve()

    def legal_files(directory:Path|str=Path.cwd(), files_2_exclude:list[str]) -> Path:
        for f in walk(Path(directory)): 
           if f.name not in files_2_exclude: 
               yield f

Note how no file directory will ever hit the recursion limit so using recursion here is fine.. Now we can simply iterate over the legal files. From here I would compile some regex, and possibly do something like below

import re

WORDS_2_EXCLUDE = ["sit", "lorem", "lipsum"]
REPLACE_LINES = re.compile(r"^.*("+"|".join(WORDS_2_EXCLUDE)+").*$")

def replace_files_in_path(path, recursive:bool=False) -> None:
    for filepath in legal_files(path, recursive=recursive):
        with open(filepath, "r") as f:
            new_content = REPLACE_LINES.sub("", f.read())
        with open(filepath, "w") as f:
            f.write(new_content)

Note that the code above is not tested, it will have bugs. The best way to learn is to fix those bugs, and understand the code. I would also to recommend implementing the recursive option so your users can choose to iterate recursively, or just search the current directory. I gave it an attempt, but make sure it actually works.

Good luck improving!

Answer 2

I find programs like yours to be excellent vehicles for improving one's coding skills. Why? Because they have the potential to be incredibly destructive. One false move and an entire directory tree can be obliterated. In my youth, on more than one occasion I wrote programs like this only to find myself frantically pressing CTRL-C moments later. Let's use that danger to our benefit.

When you're doing something dangerous, don't multitask. Your current code tries to do everything at the same time: reading paths from a directory tree; filtering out paths to be ignored; reading lines from specific files; filtering out unwanted lines; and then writing desired lines back to the files. Interspersed in all of those operations are various print operations, perhaps motivated by your sense of the risks: if we print everything we're doing, one might hope, perhaps we can convince ourselves that everything is going alright.

Separate dangerous things from safe things. Of course, most of those operations are not dangerous (reading, filtering, etc). Only the final writing is destructive. At a minimum, keep the dangerous part separate from the rest. Even better, of course, is organize the program more fully to separate the different operations from each other.

Design for partial execution. Writing, debugging, and modifying highly comingled code like this is a hassle, because every execution leads to a potentially destructive ending if you make a mistake. Recognize that risk at the beginning and design the program to allow you to run it in "safe mode". There are various ways to do that (e.g. command-line options or environment variables), but the simple idea is to provide an easy usage mechanism to control which parts of the program to execute at all or for real.

Approach one-liners with a similar mindset. You already have a useful review that includes a suggested grep-xargs-sed one-liner to do all of this destruction with a single swing of the axe. As an old-school Unix guy, I love that kind of trickeration. However, you have to treat those tools as the unruly chainsaws that they are. Approach them incrementally, with caution, and don't run the whole monstrosity on the first attempt. Instead, first convince yourself that the grep-xargs combo is correct. Then experiment with sed on a small subset of copied files. Only after you have high confidence should you turn it loose on a real directory tree (often after making a backup rsync first).

Embrace functions and data objects. If a one-liner is insufficient for your needs (it often is), the key to applying those ideas is to decompose your program into discrete functions, each focusing narrowly on a specific operation. Here's a top-level sketch. The specific implementation will depend on your precise needs for the program. This illustration uses a simple data object, a Line, to represent one line of text from file plus a flag to indicate whether the line matched any of the remove-patterns.

from dataclasses import dataclass

@dataclass
class Line:
    text: str
    matched: bool

def main(args):
    # Parse command-line arguments to control what/how to run.
    # See argparse.
    opts = parse_command_line(args)

    # Collect path of interest.
    # If you want to do printing of any kind, try to do it here (or in
    # a separate function), not inside these functions doing the core work.
    remove_patterns = read_remove_patterns(pattern_path)
    all_paths = collect_file_paths(root_path)
    relevant_paths = filter_file_paths(all_paths, ignore_files)

    # Process each file.
    for path in relevant_paths:
        lines = tuple(process_file(path, remove_patterns))

        # At this point, you can do whatever is needed
        # based on command-line options. Examples:
        #
        # - Print lines to be omitted/kept or both.
        # - Print affected/unaffected file paths.
        # - Or actually overwrite files.
        #
        # Of course, do those things in their own functions as well.
        ...

def process_file(path, remove_patterns):
    for line in read_file(path):
        matched = line_matches(line, remove_patterns)
        yield Line(line, matched)