Skip to content

add excludeFuzzy option - #4

Open
uriesk wants to merge 1 commit into
ttag-org:mainfrom
uriesk:main
Open

add excludeFuzzy option#4
uriesk wants to merge 1 commit into
ttag-org:mainfrom
uriesk:main

Conversation

@uriesk

@uriesk uriesk commented Mar 9, 2024

Copy link
Copy Markdown

Add loader option to exclude fuzzy labeled translations.

Default is to include fuzzy for backwards compatibility.

Comment thread index.js
Comment on lines +9 to +10
for (const ctxt of Object.values(poData.translations)) {
for (const msgid of Object.keys(ctxt)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't recall why I used that specific idiom...

  • maybe it was specifically to drop header comments
  • maybe because of ES level?
  • or maybe I was lazy?

Please check that the semantics are still the same 🙇🏻

Comment thread index.js

module.exports = function (source) {
if (this.cacheable) this.cacheable();
const excludeFuzzy = this.getOptions().excludeFuzzy;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this.getOptions is available since Webpack 5

so says someone on stack overflow.. that should be good enough, I think?

Comment thread README.md
Comment on lines +31 to +39
To exclude translations with the fuzzy flag, add option `excludeFuzzy` to loader:

```js
// 1. Tell webpack how to load PO files
config.module.rules.push({
test: [/\.po$/],
loader: "ttag-po-loader",
test: [/\.po$/],
loader: "ttag-po-loader",
options: {
excludeFuzzy: true,
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have a slight preference to documenting options in a more generic way, for example, use this code example and list the possible options:

config.module.rules.push({
  test: [/\.po$/],
  loader: "ttag-po-loader",
  options: {
    excludeFuzzy: true,
  },
});

Available options:

  • excludeFuzzy: blah blah blah long description what it does

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd also move the options bit below CRA, maybe...

  • if CRA caveat is still valid, which is then rather important for new users.
  • if excludeFuzzy is a rare case.

However, if you recon that exceludeFuzzy should be the default, then let's make exclusion default and add inclusion via a flag.

Comment thread index.js
Comment on lines +13 to +14
if (excludeFuzzy && msg.comments.flag
&& msg.comments.flag.includes('fuzzy')) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was thinking if conditional chaining could be used, but that's only available since 2020, so maybe it's better to stay safe like this...

@dimaqq dimaqq left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good overall :)

@dimaqq

dimaqq commented Apr 16, 2024

Copy link
Copy Markdown
Collaborator

P.S. I don't have a setup where I can test this code 🙈

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.

2 participants