eli64s / readme-ai

README file generator, powered by large language model APIs 👾
MIT License
1.34k stars 143 forks source link

fix extra language variable #110

Open Zeredbaron opened 3 weeks ago

Zeredbaron commented 3 weeks ago

Removes extra language variable from quickstart.py

Zeredbaron commented 3 weeks ago

r? @eli64s

eli64s commented 2 weeks ago

Hi @Zeredbaron, thanks for the PR! I just did some testing to ensure we're handling edge cases correctly, specifically when language is None or an empty string ''.

Example 1 - Your PR #110

Example 1
Behavior: Removing the first conditional check results in `None` and empty string values being added to `language_counts`. Code: ```python blacklist = [".git", ".gitignore", ".md", ".txt", ".yml", ".yaml", ".json"] languages = [None, ".py", ".js", ".md", ".json", None, None, "", '', "", " "] language_counts = {} for language in languages: if language not in blacklist: language_counts[language] = language_counts.get(language, 0) + 1 print(f"Example 1: language_counts: {language_counts}") ``` Output: ```python language_counts = { None: 3, '.py': 1, '.js': 1, '': 3, ' ': 1, } ``` This results with a `None` key and empty string `''` key included in the returned dictionary. This may lead to incorrect/incomplete information in the output README file.

Example 2 - Current Implementation

Example 2
Behavior: The first check (`if language`) ensures `None` and empty strings are not counted, which is the intended behavior. Code: ```python blacklist = [".git", ".gitignore", ".md", ".txt", ".yml", ".yaml", ".json"] languages = [None, ".py", ".js", ".md", ".json", None, None, "", '', "", " "] language_counts = {} for language in languages: if language and language not in blacklist: language_counts[language] = language_counts.get(language, 0) + 1 print(f"Example 2: language_counts: {language_counts}") ``` Output: ```python language_counts = { '.py': 1, '.js': 1, ' ': 1, } ``` `None` or an empty string `''` values are correctly skipped, but strings with only whitespace are counted, which is a new bug!

Additional Notes

This analysis suggests the current implementation works as intended, excluding all None and empty string '' values. However, it looks like we found a new issue as the function does not properly handle empty strings that contain one or more whitespaces ' ' or " " .

This issue is not related to your PR, but would be good to fix to take a look at! We'll need to update the count_languages method on line 38 to strip whitespace and check for empty strings. This should ensure only valid language entries are counted in the returned dictionary.

Let me know if you have any questions or if you'd like to tackle this improvement 🙂

Eli