-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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 bug wrong metadata on translated docs
#709
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
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)) { | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you want to pass e.g., on line 219 in this file:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only need to specify the language case, if we use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if (!res) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
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 belanguage
Save one line of code ? 😃
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean
instead of
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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 ☺