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

#61: Remove main language in languages list #86

Conversation

valimero
Copy link

@valimero valimero commented Mar 8, 2023

I quick fixed this problem: #61

@valimero valimero force-pushed the fix-duplicate-language-with-main-language branch from 996c059 to c313520 Compare March 9, 2023 10:59
@joshuatz joshuatz self-requested a review March 21, 2023 06:45
Copy link
Owner

@joshuatz joshuatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @valimero!

Thanks for making a PR - I always appreciate contributions to the project. Sorry for taking a while to get around to reviewing.

Unfortunately, I'm not sure I understand how your PR fixes the underlying problem discussed in the linked issue (#61). Commenting out the code that combines the main profile language with the manually / explicitly set languages simply ignores the main profile language(s) completely and only uses the manually listed ones.

Side note: In general, it is better to completely delete code instead of commenting-out. With git, we always have the full history we can pull from at any time 😎

I feel like this code change might not cover some of the edge-cases discussed over in #61 and why I didn't make this change myself already.

For example, what if a user has their main language set to Polish, but for their manually listed languages, they only included English, not Polish and English, because they assumed that people reading their resume would know they speak Polish, but English is an "extra skill" for them, so they manually listed it. With your code change, the only language that would show up on the resume is Polish.

If you think there is something I am overlooking or missing, I am happy to discuss further!

@joshuatz joshuatz closed this Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants