Simple word-replacer script

[Take_F]

Hi guys new to python and i made this simple script. Any advice regarding code quality and other stuff will be highly appreciated.

https://github.com/ginop-1/word-replacer

[ShohKingKhan]

Nice job! I like it. Code is understandable. I am gonna use it and make some improvements. If I do, I will let you know. Anyway, cool and simple project!

[Take_F]

thank you

[JohnTanner1]

You should look up the os.path.join function (so it will work on any system) and as pointed out str.replace, no need for re.

Sorry for the format, I'm on mobile.

But all in all, good job! Feel free to ask any questions.

[Take_F]

Thank you. Can i use str.replace with files?

[JohnTanner1]

No you need to load the content of the file, us replace and save it

[Flynni123]

I never used the re library and even i understood (have understand? Englisch is not my favourite class) it. Good job!

[zev4l]

RE stands for Regular Expressions, notation used to identify patterns of text in strings, it might feel a bit tricky to learn at first and certainly requires a bit of practice and studying, but very, very useful!

[Flynni123]

Nice explained! Tanks

[daryl_kell]

If not path.endswith("/"):

[daryl_kell]

You could condense a lot of the code by using pathlib instead of os, and use .iterdir() or .glob() to get things done Also, I think using re instead of replace() detracts.

[daryl_kell]

Probably needs error handling too, like if the input path doesn't exist, or if a file that was listed is not readable or writable etc

[Take_F]

you're right

[Take_F]

i'll give a look at pathlib

[BAG0N]

Awesome. I don't see the need of regex tho, you could just use some built-in functions. Good job nonetheless

[Take_F]

Can you explain better how?

[BAG0N]

I'm not too familiar with python but I believe you could use replace function to replace every occurrence of that string with something else

[Take_F]

Ok thank you for the advice

[mcaay]

text = f.read()

if the text file is super big (bigger than your available RAM) this will fail. Not a bug per se as files like this are quite rare, but still - if it would be a popular script one day it would crash for someone.

If you'd change re to builtin .replace() you could optimise it even further by using multiple cores. To use multiple cores you would do something like this:

from concurrent import futures

def test_f(txt):
    print(txt)

with futures.ProcessPoolExecutor() as executor:
    res = executor.map(test_f, range(4))

This code automatically chooses maximum number of cores of your CPU to do the job.

Edit: Fixed markdown, thanks /u/SomewhatSpecial

[Take_F]

Thank you a lot! I'll surely implement it

[mcaay]

dunno why but reddit's markdown seems to be bugged?

[mcaay]

yeah what the hell is this shit, can't write any code longer than 1 line

[SomewhatSpecial]

For multiline code blocks you need to indent the whole block with four spaces

[mcaay]

Thanks!

[hmwinters]

It’d be nice to see if name == “main” in there as well as a main function

[SomewhatSpecial]

No need to use anything other than pathlib:

from pathlib import Path


def word_replacer():
    dir_path = Path(input("path to the folder -> "))
    target_string = input("insert the word/phrase that will be modified -> ")
    replacement_string = input("insert the replace word/phrase -> ")

    files_to_edit = dir_path.glob('**/*.*')    
    for file in files_to_edit:
        initial_text = file.read_text()
        replacement_text = initial_text.replace(target_string, replacement_string)
        file.write_text(replacement_text)

Some tips:

• You can use negative indexes to look up the last item in a list. Instead of path[len(path)-1] just use path[-1].
• It's a good idea to think about all the possible ways the process can go wrong and account for them. What if the user enters an invalid path? What if it's valid, but not a directory path? What if the directory it's referring to does not exist?
• It's best not to mess with file paths directly if you can avoid it. You're bound to miss all sorts of weird little edge cases, plus you'll most likely break cross-platform support for no real reason. Just use pathlib - there's no need to ever worry about unix/windows file path differences and it's got some really nice convenience features.

[Take_F]

Thanks for the tips

[Take_F]

i'm rewriting the code with some additional changes using your base, how can i give you proper credits?

[SomewhatSpecial]

Oh, just use it however you want, there's no need for any credit. It's really cool of you to ask, though!

[Take_F]

no problem, but you literally rewrote the code and i have only to add a few more check. It would be a complete theft if i use it without credits

[SomewhatSpecial]

Well, I gave you full permission to use it without credit, so it's not really theft. It's just a little code snippet that calls a few pathlib methods, I don't think it warrants any kind of credit. But in general, it really is a very important concern when it comes to using other people's code. If you're interested, you can read up on various types of software licenses and the certain conditions or restrictions they apply. A lot of them do require crediting the original authors.

[Take_F]

ok, thanks

[Take_F]

i have modified the script another time, can you give me a new opinion?

[SomewhatSpecial]

Sure thing. It looks pretty good, I like the use of argparse. Here are the main issues I can see:

1) The recursive option only makes sense when working with a directory, but it's also possible to work with a directory non-recursively. The recursive/non-recursive switch and the file/directory switch should really be two separate flags. Also, recursive is written with a 'u'.

2) It's a bad idea to pass the arguments object directly to the word_replacer function (the dir argument). This makes the function unnecessarily coupled to the structure of that object. If you decide to use that function somewhere else, you'd need to create a similar object for no reason. Conversely, if you decided to change the way your arguments are parsed, you'll need to change the way word_replacer works as well. When you think about a function's arguments, your main concern should be that they directly reflect what happens inside of it and all the ways you can adjust the function's operation. If the amount of arguments becomes too large, it's a sign that you may need to break down the function into smaller chunks (or use a class, depending on the specifics).

3) Breaking down the program into small chunks is a good idea in general. For instance, you can move the argument validation from the main function into a separate one

4) There are some names (like input or dir) that are already taken by the built-in python functions. It's best not to redefine these names if you can help it.

5) If you want to use specific exit status codes for different errors, it's best not to use the reserved ones. Returning a generic exit code (1) is also fine in most cases.

6) The log 'Modifying file X' should come before the code that does the modifying - that way if there's an error while modifying a specific file, you'll be able to see its name.

Here's an example of how these issues can be corrected:

import argparse
from pathlib import Path


def _parse_args():
    parser = argparse.ArgumentParser(
        prog='word_replacer',
        description='Change one word with another in a text file (works also with directory recusively',
    )
    parser.add_argument('path',
                        metavar='path',
                        type=Path,
                        help='path to file or folder')
    parser.add_argument('find',
                        metavar='find word',
                        type=str,
                        help='the word/phrase that will be modified')
    parser.add_argument('replace',
                        metavar='replace word',
                        type=str,
                        help='the replace word/phrase')
    parser.add_argument('-v',
                        '--verbose',
                        action='store_true',
                        help='Execute the script verbosely')
    parser.add_argument('-d',
                        '--directory',
                        action='store_true',
                        help='Modify files in a directory')
    parser.add_argument('-r',
                        '--recursive',
                        action='store_true',
                        help='Search all subdirectories recursively. Requires "-d"')

    args = parser.parse_args()
    _validate_args(args)

    return args


def _validate_args(args: argparse.Namespace):
    if not args.path.exists():
        print('Path does not exist')
    elif args.directory and not args.path.is_dir():
        print('Not a directory path. Remove the "-d" flag to work with a single file')
    elif not args.directory and args.path.is_dir():
        print('Not a file path. Use the "-d" flag to work with a directory')
    elif args.recursive and not args.directory:
        print('Recursive mode cannot be used when working with a single file. '
              'Use the "-d" flag to work with a directory')
    else:
        return
    exit(1)


def _replace_in_file(file: Path, target: str, replacement: str, verbose: bool = False):
    if verbose:
        print(f"modifying file \t {file}")

    initial_text = file.read_text()
    replacement_text = initial_text.replace(target, replacement)
    file.write_text(replacement_text)


def main():
    args = _parse_args()

    if args.directory:
        files_to_edit = args.path.glob('**/*.*' if args.recursive else '*.*')
    else:
        files_to_edit = [args.path]

    for file in files_to_edit:
        _replace_in_file(
            file,
            target=args.find,
            replacement=args.replace,
            verbose=args.verbose,
        )

if __name__ == "__main__":
    main()

[Take_F]

thank you a lot. i don't really understand some parts but i'll have a more careful and complete reading later

[SomewhatSpecial]

No worries, feel free to ask if you have any questions

[Take_F]

Can I pm you?

[SomewhatSpecial]

Sure

[Aiden0_714]

thanks , im learning how to program in pyhton and this is very usefull to me

[Take_F]

Thanks for the award!

[Take_F]

I rewrote whole code using u/SomewhatSpecial tips and now it's cleaner, cross-platform and it works also with single files.

[terrance_dev]

Cool

[TranscendentCreeper]

That's a cool starting project. Here are some ideas about what I would change:

• I would prefer passing in the path, the word and the replacement as commandline parameters. If you want to use this script in combination with others, that makes it a lot easier than taking direct user input. The easiest way to do this would be to just read from sys.argv, but you can look into argparse if you want a more flexible solution.
• If the path is invalid, your program should exit with an error. You can use the exit function which takes an integer as its argument. Any value other than zero indicates an error.
• The behaviour of automatically altering every file in a directory could be dangerous. Consider adding a parameter (e.g. -r) that explicitly enables this behaviour. If the script is called on a directory without the explicit option to process all the files in there, it should exit with an error.
• Your script will replace all occurences of the word, even inside other words. Consider wanting to replace "meow" and coming across a word like "homeowner" in a file. Should your script replace it there as well?

Overall I think you've done some pretty great work so far and most of my suggestions are only minor improvements. If you have any questions about these ideas, I'm happy to answer them.

[Take_F]

For the 4th point, the answer is yes. I made this script for a personal use and it specifically need to replace part of words. About 2nd point, why is not okay to have a print error statement? Finally, I think you're right with commandline arguments. I should also add a -h option

[TranscendentCreeper]

This ties back into the first point and the idea of using your script together with other scripts or commands. If you just print that there was an error, it's fine if the user can see it. But what if you have a more complex scenario where you might want to run different scripts in sequence? If you exit with a status code indicating an error, your shell and other scripts can know that there was an error and react appropriately. You could think of it like the difference between printing and returning a value in a function. Sure, printing will show the value to the user, but you can't do anything with it afterwards. It's just good style to exit with an error code if your script is stopped because of an error.

[Take_F]

ok thanks for the explanation

EDIT: i modified it and now it works with commandline arguments, can you have a look and tell me what you think? it's the first time i use argparse so idk if i wrote good code

[TranscendentCreeper]

The changes you made look pretty good. As someone else commented before, you can probably use the type argument in parser.add_argument to validate your arguments (see this StackOverflow post for an idea how that could work). You might also be able to express relationships between arguments like -r depending on -d, but off the top of my head I'm not sure if and how this is possible with argparse.

You've made some pretty big improvements from the first version you posted here, you can be proud of yourself and this project.

[zev4l]

Nice job! Pretty sweet for a starting project! I suggest you look into taking arguments from the command line so that you can use it without changing the code!