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.

Sign up for GitHub

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

Jump to bottom

Fix bug wrong metadata on translated docs #709

Closed
wants to merge 3 commits into from
Closed

Fix bug wrong metadata on translated docs #709

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d02f22
Fix bug wrong metadata on `translated docs`
gimdongwoo May 29, 2018
66cef23
fix related review
gimdongwoo May 30, 2018
45f16c8
Merge branch 'master' into metadata
gimdongwoo May 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions lib/server/readMetadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,18 @@ function extractMetadata(content) {
}

// process the metadata for a document found in the docs folder
function processMetadata(file) {
function processMetadata(file, _language) {
const result = extractMetadata(fs.readFileSync(file, 'utf8'));

let regexSubFolder = new RegExp(
'/' + escapeStringRegexp(getDocsPath()) + '/(.*)/.*/'
);
let language = _language;
Copy link
Contributor

@endiliey endiliey May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

why don't we delete this proposed line 128 and replace _language at proposed line 125 to be language
Save one line of code ? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This 3 lines is moved to the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the code is not needed in this situations if language is given.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean

function processMetadata(file, language) {

instead of

function processMetadata(file, _language) {
let language = _language;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no-param-reassign is good code style.
https://eslint.org/docs/rules/no-param-reassign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course if you want to change it, I will change it. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually had that doubt as well. I'll leave it to @yangshun & @JoelMarcey to decide ☺

if (!language) {
let regexSubFolder = new RegExp(
'/' + escapeStringRegexp(getDocsPath()) + '/(.*)/.*/'
);

const match = regexSubFolder.exec(file);
let language = match ? match[1] : 'en';
const match = regexSubFolder.exec(file);
language = match ? match[1] : 'en';
}

const metadata = {};
for (const fieldName of Object.keys(result.metadata)) {
Expand Down Expand Up @@ -271,7 +274,7 @@ function generateMetadataDocs() {
const extension = path.extname(file);

if (extension === '.md' || extension === '.markdown') {
const res = processMetadata(file);
const res = processMetadata(file, language);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to pass language in to the other calls to processMetadata?

e.g., on line 219 in this file:

const res = processMetadata(file);

Copy link
Contributor Author

@gimdongwoo gimdongwoo Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We only need to specify the language case, if we use translated_docs. In general, we do not need to specify, because we use getDocsPath. It seems to increase unnecessary processing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @gimdongwoo - In general though, I don't like leaving a parameter missing to a function call. It can cause confusion in reading the code. Even if you don't pass language -- which I still think is ok - can you pass something, even if a ''?

if (!res) {
return;
}
Expand Down