Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix extra language variable #110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Zeredbaron
Copy link

Removes extra language variable from quickstart.py

@Zeredbaron
Copy link
Author

r? @eli64s

@eli64s
Copy link
Owner

eli64s commented Jun 26, 2024

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:

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:

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:

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:

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


@eli64s eli64s added the bug Something isn't working label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants