improve font loader

[diamant3]

• use list and loop only for automatic search path of the figlet share directory
• add current share directory used when you type pyfiglet -l like this:

fix for #86

[pwaller]

Would it be better to have this behave as a search path? In other words, each of the directories in the list are searched in turn when a font is requested. That way, if the font is found in any of the directories, it is found (rather than only considering the first existing directory?).

My suggestion is to make this a list of paths, and update the logic which opens a path to loop over all the paths until a hit is found.

What do you think?

[diamant3]

Thank you for your reply! And I think the suggestion you said, that is what the code does. It loops every path in the list until it hits a path, depending on their OS and figlet installation(they can submit a PR to add a directory to search for on that list) to make it cross-platform. correct me if I'm wrong because english is not my primary language, thanks again!

[pwaller]

Hi, No problem. Happy to explain so feel free to keep asking if you still don't understand, and there is no urgency so we can take our time.

What I understand you've done is change the initialisation so that it chooses the first directory which is found as the directory which is searched. Given a font lookup, there is exactly one directory checked to see if it has that font. What I think users may prefer is if all of the directories are checked.

To achieve this I think SHARED_DIRECTORY = "..." should become SEARCHED_DIRECTORIES = [...], and whatever logic uses SHARED_DIRECTORY should be updated so that it checks each directory in turn until a font is found or there are no more directories to check. Hope that makes sense.

If I become unresponsive, please feel free to ping the thread once weekly (ideally late on a Thursday or early on a Friday).

[diamant3]

Thank you so much for your consideration and time! sure, no worries I will do that

[peterbrittain]

Note that this isn't the only place SHARED_DIRECTORY is used. You will need to fix up all references to it. In particular, the logic in installFont() will need to pick a specific option from the list.

[diamant3]

Thank you for the reply! Yes, I took note of that the SHARED_DIRECTORY is used even inside the methods of the classes, but I don't think that it can affect their logic. The code is still the same as before, except for the logic of automatic search.

By the way please bear with me, I'm just learning about your codebase and always open to learn from more experienced people. Thanks again for the note you gave!

[pwaller]

@diamant3 I think you've not understood what is being asked for.

What @peterbrittain and myself are thinking is that there needs to be a function, findfont(...) which returns the path to a font. The way this function works is by consulting each directory in turn. Anything which previously used SHARED_DIRECTORY should use this new function.

There shouldn't be any module-level execution, only a module level variable, SEARCHED_DIRECTORIES.

Does that make sense, and do you see how this is different from what you have implemented so far?

[diamant3]

I'm sorry, Yes it's so huge different on my implementation, thank you so much! I will rewrite this and make an update next week.

[diamant3]

Hello, Can i request a review and test my PR, thank you!

[peterbrittain]

Looks good! Just a few quick tweaks please...

[diamant3]

Thank you so much! sure no worries, just a minute

edit: it's done, thanks!

[pwaller]

Sorry if I wasn't clear before but this is still not quite what I envisaged. You've made this into a function which gets called when it's needed, that's good; but the function needs to take a font to search for as a parameter. Because the font may not exist in "the first directory found", but may exist in a later one, and different fonts may be found in different directories.

Listing all fonts should likewise consult all directories and return the union of all fonts installed in all directories. Does what I'm asking for seem reasonable?

[peterbrittain]

Listing all fonts should likewise consult all directories and return the union of all fonts installed in all directories. Does what I'm asking for seem reasonable?

Yeah - I wondered about this, but stuck with simplicity. Happy for you to override that as owner of the project, though.

I see two issues:

1. The logic to list the fonts will need replacing to create a unified set of fonts.
2. The logic to install a font needs to pick a directory where the font shouldn't already exist.

Both possible to do, but both will need testing.

[diamant3]

That's on me, my bad. Yes, that's reasonable no worries. So this findFont(font) function with a parameter font will gather the font found in every directory, but what do I need to return if it exists? the directories it found with a font like this /usr/local/pyfiglet/<font> or just a path to that directory like this one /usr/local/pyfiglet/ for example, and it will result in a list of directories if it exists in another directory, am i right?

Happy for you to override that as owner of the project

Thank you so much for the permission!, and got it that issues i need to take note and needs to change according to findFont function, am i right?

[pwaller]

It's not necessarily one function. You need to consider three pieces of functionality of the software:

1. Normal running of pyfiglet (rendering text). This needs to find a font. My ideal implementation of this would change whatever the current logic is for 'open(font)' and instead do something analogous to 'loadFont(font)' -- under the hood this function would iterate over each directory attempting to open fonts and return as soon as it has one which does not raise a FileNotFoundError.

2. Listing fonts. This needs to make a set containing all available fonts. This should operate by calling os.listdir on each of the paths, ignoring paths which don't exist.

3. Installing fonts. In the existing version, this installs to SHARED_DIRECTORY, which is /usr/local/share/pyfiglet on non-win32. Retaining this existing behaviour is OK, and as @peterbrittain says maybe we actually don't want to consider other paths for installation since those aren't "managed" by pyfiglet.

[peterbrittain]

Heavy plus 1 for what @pwaller said!

[diamant3]

Hello, so I implement the logic from pwaller's said to my best understanding, can i request again for review and as always, thank youu.

[pwaller]

Apologies the ball got dropped on this one. I see you marked the PR as draft (rather than ready to review), is that your intended state? I've left another comment.

[pwaller]

This previously checked $PWD as well as SHARED_DIRECTORY. I think we should preserve the existing behaviour.