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

Removing html angular extractor for issue 4018 #4680

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

zashary
Copy link
Contributor

@zashary zashary commented Aug 4, 2023

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

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #4680 (919ff86) into main (d7c5577) will decrease coverage by 0.04%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            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     
Flag Coverage Δ
Linux_1 34.84% <ø> (ø)
Linux_2 55.13% <ø> (ø)
Linux_3 43.40% <ø> (+<0.01%) ⬆️
Linux_4 34.96% <100.00%> (-0.15%) ⬇️
Windows_1 34.85% <ø> (ø)
Windows_2 55.10% <ø> (ø)
Windows_3 43.40% <ø> (ø)
Windows_4 34.96% <100.00%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/dev/i18n/extract_default_translations.js 90.62% <100.00%> (-0.56%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this change is causing the unit test to fail. reverting it should make your tests pass.

Copy link
Member

Choose a reason for hiding this comment

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

yarn test:jest <path to schema_error.test.ts> -u
-u could update the snapshot

Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Member

Choose a reason for hiding this comment

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

For Changelog, we mean add this PR in CHANGELOG.md. You could check other PRs for a reference.

@@ -135,6 +135,7 @@
# 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"
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this 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.

This was part of testing. I've gone ahead and removed it.

Copy link
Member

Choose a reason for hiding this comment

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

yarn test:jest <path to schema_error.test.ts> -u
-u could update the snapshot

}
paths.codeEntries.push(resolvedPath);

// if (resolvedPath.endsWith('.html')) {
Copy link
Member

Choose a reason for hiding this comment

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

could just remove this part.

Copy link
Member

Choose a reason for hiding this comment

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

nit: should we do

const codeEntries = entries.reduce(
    (paths, entry) => {
      const resolvedPath = path.resolve(inputPath, entry);

      paths.codeEntries.push(resolvedPath);
      return paths;
      },
     [] 
  );

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good call out and looks a lot cleaner that way! I've gone ahead and updated this bit.

@@ -29,4 +29,4 @@
*/

export { extractCodeMessages } from './code';
export { extractHtmlMessages } from './html';
//export { extractHtmlMessages } from './html';
Copy link
Member

Choose a reason for hiding this comment

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

should we just remove this line?

@zashary zashary force-pushed the 4018 branch 3 times, most recently from 434777b to b4b0f4f Compare August 22, 2023 00:01
@zashary zashary changed the title [WIP] Removing html angular extractor for issue 4018 Removing html angular extractor for issue 4018 Aug 22, 2023
@zashary zashary marked this pull request as ready for review August 22, 2023 16:50
Signed-off-by: Zashary Maskus-Lavin <zashary.maskus-lavin@oracle.com>
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM :) Thanks for the contribution @zashary

@ananzh ananzh merged commit 72ced4d into opensearch-project:main Aug 31, 2023
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants