Removing html angular extractor for issue 4018#4680
Removing html angular extractor for issue 4018#4680ananzh merged 1 commit intoopensearch-project:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4680 +/- ##
==========================================
- Coverage 66.30% 66.27% -0.04%
==========================================
Files 3322 3321 -1
Lines 63919 63831 -88
Branches 10115 10115
==========================================
- Hits 42381 42301 -80
+ Misses 19050 19049 -1
+ Partials 2488 2481 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ashwin-pc
left a comment
ashwin-pc
left a comment
There was a problem hiding this comment.
The change looks good to me :) can you make sure that you sign the commit and add a changelog entry for the PR and revert the change to the schema_error.test.ts snapshot? With that i think we can merge this PR while you work on deangularizing i18n
There was a problem hiding this comment.
Looks like this change is causing the unit test to fail. reverting it should make your tests pass.
There was a problem hiding this comment.
yarn test:jest <path to schema_error.test.ts> -u
-u could update the snapshot
There was a problem hiding this comment.
For sign the commit in the future, you could do
git config --global user.name "Your name"
git config --global user.email your@email.com
Then for commit: git commit -sm "<commit msg>"
For fixing this one, you could amend the commit directly by adding the sign-off. For example
git commit --amend
Then add
Signed-off-by: zashary <your email>
There was a problem hiding this comment.
For Changelog, we mean add this PR in CHANGELOG.md. You could check other PRs for a reference.
| # Specifies locale to be used for all localizable strings, dates and number formats. | ||
| # Supported languages are the following: English - en , by default , Chinese - zh-CN . | ||
| #i18n.locale: "en" | ||
| i18n.locale: "es" |
There was a problem hiding this comment.
This was part of testing. I've gone ahead and removed it.
There was a problem hiding this comment.
yarn test:jest <path to schema_error.test.ts> -u
-u could update the snapshot
| } | ||
| paths.codeEntries.push(resolvedPath); | ||
|
|
||
| // if (resolvedPath.endsWith('.html')) { |
There was a problem hiding this comment.
nit: should we do
const codeEntries = entries.reduce(
(paths, entry) => {
const resolvedPath = path.resolve(inputPath, entry);
paths.codeEntries.push(resolvedPath);
return paths;
},
[]
);
?
There was a problem hiding this comment.
That's a good call out and looks a lot cleaner that way! I've gone ahead and updated this bit.
|
|
||
| export { extractCodeMessages } from './code'; | ||
| export { extractHtmlMessages } from './html'; | ||
| //export { extractHtmlMessages } from './html'; |
There was a problem hiding this comment.
should we just remove this line?
zashary
force-pushed
the
4018
branch
3 times, most recently
from
434777b to
b4b0f4f
Compare
zashary
changed the title
zashary
requested review from
AMoo-Miki,
BSFishy,
Flyingliuhub,
ZilongX,
abbyhu2000,
curq,
joshuarrrr,
kavilla,
kristenTian,
manasvinibs,
seanneumann,
zengyan-amazon and
zhongnansu
as code owners
Signed-off-by: Zashary Maskus-Lavin <zashary.maskus-lavin@oracle.com>
3 participants
Description
This PR removes the angular html extractor for dashboards and is a part of the larger de-angular effort.
Part of work to address #4018
Issues Resolved
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr