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 Headers parsing function #38

Merged
merged 3 commits into from
May 18, 2017
Merged

Conversation

zbraniecki
Copy link
Collaborator

Fixes #34

if (typeof string !== 'string') throw new TypeError('Argument must be a string');
const tokens = string.split(',').map(t => t.trim());
return tokens.filter(t => t !== '').map(t => {
return t.split(';')[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't take into effect the q-value nor the specificity of the locale tag. Are we fine with this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this is on purpose. We don't act upon the q-value at all. The order is the only indication for our fallback, there's no increase/decrease in fallback based on the q-value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to extend this in the future? The RFC says the q-values are in fact important.

Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

We'll also need to re-export this in src/index with:

export { default as acceptedLanguages } from './headers';

@@ -0,0 +1,8 @@

export default function parseAcceptedLanguagesHeaderIntoArray(string = '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this simply to acceptedLanguages please.

@@ -0,0 +1,8 @@

export default function parseAcceptedLanguagesHeaderIntoArray(string = '') {
if (typeof string !== 'string') throw new TypeError('Argument must be a string');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please break this into an if .. and a {} block.

@stasm
Copy link
Contributor

stasm commented May 18, 2017

And maybe rename the file to accepted_languages.js :)

@zbraniecki
Copy link
Collaborator Author

Applied feedback

@@ -0,0 +1,10 @@

export default function aceptedLanguages(string = '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: s/acepted/accepted/.

const tokens = string.split(',').map(t => t.trim());
return tokens.filter(t => t !== '').map(t => {
return t.split(';')[0];
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nit, but there's no need to use {} here and an explicit return. Can you change this to a one-liner arrow function?

@zbraniecki zbraniecki merged commit 5181864 into projectfluent:master May 18, 2017
@zbraniecki zbraniecki deleted the headers branch December 12, 2017 04:43
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.

None yet

2 participants