scripts/_common.py: add a shared Python file to move duplicated code

[sebastiaanspeck]

#12745 (comment)

[vitorhcl]

I was going to open a issue myself and start the refactoring another day, thank you for already starting this work :)

I took a quick look at the code, and in general it looks good indeed.

I think we can extract more functions, for example: the "--page" code in main(), I think we can extract the gathering of the target paths and maybe more things.

I will see if I can review this PR more carefully later.

[sebastiaanspeck]

I was going to open a issue myself and start the refactoring another day, thank you for already starting this work :)

I took a quick look at the code, and in general it looks good indeed.

I think we can extract more functions, for example: the "--page" code in main(), I think we can extract the gathering of the target paths and maybe more things.

I will see if I can review this PR more carefully later.

It is WIP 🚧 and definitely not ready to merge for the time being, but feel free to add commits 😄

[vitorhcl]

(I didn't took a closer look at the code yet)

Unfortunately it's not possible to comment on unmodified lines, but set-alias-page.py still depends on os.path, it uses some of its functions in, for example get_alias_page() and set_alias_page(), for example.

It would be good to remove the os.path and use pathlib everywhere since we are already modifying the scripts, and also because _common.py uses pathlib.

[sebastiaanspeck]

It would be good to remove the os.path and use pathlib everywhere since we are already modifying the scripts, and also because _common.py uses pathlib.

I will definitely look into that. I found out yesterday that the set-alias-page.py as it is right now in this PR does not skip the files that are listed in the IGNORE_FILES

[vitorhcl]

I will definitely look into that. I found out yesterday that the set-alias-page.py as it is right now in this PR does not skip the files that are listed in the IGNORE_FILES

Does this problem also happens with set-more-info-link.py? It isn't because of the value of IGNORE_FILES, but because of the functions that use IGNORE_FILES, right?

Do you know which function(s) is/are causing this error?

[sebastiaanspeck]

I will definitely look into that. I found out yesterday that the set-alias-page.py as it is right now in this PR does not skip the files that are listed in the IGNORE_FILES

Does this problem also happens with set-more-info-link.py? It isn't because of the value of IGNORE_FILES, but because of the functions that use IGNORE_FILES, right?

I did not check. Due to the fact that set-more-info-link.py is not changing any pages at the moment, I did not look closer at that one.

Do you know which function(s) is/are causing this error?

It is on line 237 and 243 in set-alias-page.py. I think because page is not a string but a Path, checking if it is in IGNORE_FILES will always fail.

[sebastiaanspeck]

I will definitely look into that. I found out yesterday that the set-alias-page.py as it is right now in this PR does not skip the files that are listed in the IGNORE_FILES

Does this problem also happens with set-more-info-link.py? It isn't because of the value of IGNORE_FILES, but because of the functions that use IGNORE_FILES, right?

I did not check. Due to the fact that set-more-info-link.py is not changing any pages at the moment, I did not look closer at that one.

Do you know which function(s) is/are causing this error?

It is on line 237 and 243 in set-alias-page.py. I think because page is not a string but a Path, checking if it is in IGNORE_FILES will always fail.

It was a typo after all... if p not in IGNORE_FILES should've been if p.name not in IGNORE_FILES

[vitorhcl]

I have a suggestion to clarify the color handling that would require updating all code that uses any ANSI code.

[vitorhcl]

Now we use pathlib, it's easy to resolve relative paths to absolute paths.

This allow us to remove the dependency on the current directory, specially on the stage function.

BTW, adding tests for _common.py was an excellent idea. Good choice on pytest, too :)

[vitorhcl]

Continuing the modification of stage's parameters.

[vitorhcl]

A minor suggestion on the get_tldr_root error message that makes it clearer for anyone not used with environment variables, allowing them to search for the term.

[vitorhcl]

Now that we use absolute paths in stage(), there's no reason to use relative paths in these places.

Obviously, we need to update the variable names and not take the relative path

[kbdharun]

LGTM, thanks for your contribution. Tested all the changes locally.

[vitorhcl]

LGTM, thank you for your contribution.