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

add language() and languages() #11

Merged
merged 7 commits into from Aug 12, 2015
Merged

add language() and languages() #11

merged 7 commits into from Aug 12, 2015

Conversation

mark-bradshaw
Copy link
Contributor

This provides parsing of the Accept-Language header, with a language() function to determine the proper language to respond with, and languages() to return back an ordered list of acceptable languages.

@mark-bradshaw mark-bradshaw added the feature New functionality or improvement label Jul 11, 2015
@mark-bradshaw mark-bradshaw added this to the 1.2.0 milestone Jul 11, 2015

exports.charset = CharsetLib.charset;
exports.charsets = CharsetLib.charsets;

exports.encoding = EncodingLib.encoding;
exports.encodings = EncodingLib.encodings;

exports.language = LanguageLib.language;
exports.languages = LanguageLib.languages;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to have the Lib suffix on this variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't I'm going to have lines like this:
exports.language = Language.language;

Seems like a differentiator or some sort is a bonus for readability. It could be something else, but "lib" fits the bill and is short.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see what's wrong with the above. Looks better to me.

@mark-bradshaw
Copy link
Contributor Author

I've made the code style updates. I'm still trying to get the Hapi code style nailed down in my brain to spot these things. It's rough going between different projects with different styles. I'm punting for now on the map vs for loop question, but I've made an issue for it to determine whether there's an issue there or not, and I'll circle back around if there is.

Anything else?

@hueniverse
Copy link
Contributor

I only reviewed style. If you want to get someone to review logic, that might be a good idea.

@mark-bradshaw
Copy link
Contributor Author

@nlf Nathan, can you give the code here a once over? I believe the tests cover the desired output.

.map(internals.getParts)
.filter(internals.removeUnwanted)
.sort(internals.compareByWeight)
.map(internals.partToLanguage);
Copy link
Member

Choose a reason for hiding this comment

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

i feel like this could use some optimizing.. for now it's fine, but it's something to think about since it is something that potentially runs for every request.

@mark-bradshaw
Copy link
Contributor Author

Thanks Nathan. I'll leave this PR handing until I can come back and remove some of the functional programming where it makes sense.

@mark-bradshaw mark-bradshaw mentioned this pull request Jul 13, 2015
@mark-bradshaw
Copy link
Contributor Author

Decided instead to merge now and do a follow up issue for converting to functional style. I think that will work easier.

mark-bradshaw added a commit that referenced this pull request Aug 12, 2015
add language() and languages()
@mark-bradshaw mark-bradshaw merged commit ee84359 into master Aug 12, 2015
@mark-bradshaw mark-bradshaw deleted the language branch August 12, 2015 15:11
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants