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

feat: exclusion file and tests #5061

Merged
merged 4 commits into from
Dec 1, 2021

Conversation

rudouglas
Copy link
Contributor

This adds the ability to exclude files under specific paths, and of specific types which are configured using a YAML file.

This also adds tests to the add-files-to-translation-queue workflow as there weren't any before, and adds tests for excluding files/types

@rudouglas rudouglas requested a review from a team as a code owner November 29, 2021 15:34
@rudouglas rudouglas requested review from moonlight-komorebi and aswanson-nr and removed request for a team November 29, 2021 15:34
@github-actions
Copy link

Hi @rudouglas 👋
Thanks for your pull request! Your PR is in a queue, and a writer will take a look soon. We generally publish small edits within one business day, and larger edits within three days.

Gatsby Cloud will automatically generate a preview of your request, and will comment with a link when the preview is ready (usually 20 to 30 minutes).

@github-actions github-actions bot added this to Hero to triage in Docs PRs and Issues Nov 29, 2021
@rudouglas rudouglas linked an issue Nov 29, 2021 that may be closed by this pull request
2 tasks
@rudouglas rudouglas removed this from Hero to triage in Docs PRs and Issues Nov 29, 2021
@rudouglas rudouglas added this to In review in Developer Enablement Team via automation Nov 29, 2021
@rudouglas rudouglas added the eng issues related to site functionality that requires engineering label Nov 29, 2021
@rudouglas rudouglas removed this from In review in Developer Enablement Team Nov 29, 2021
Comment on lines 39 to 41
const getExclusions = () => {
return yaml.load(fs.readFileSync(path.join(process.cwd(), EXCLUSIONS_FILE)));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this can just be moved into excludeFiles, or still be kept as a separate method but called directly from excludeFiles.

additionally, i think its cleaner & would allow for us to simplify the signature of excludeFiles, since we wouldn't need to pass exclusions parameter anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only trade-off with that is we would be running yaml.load and fs.readFileSync for every file, would it not make more sense to only perform that action once?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you run it like this, would it run more than once? i might've missed a loop somewhere, is this what you meant @nr-kkenney? :

const excludeFiles = (fileData) => {
  const exclusions = getExclusions();

  return fileData.filter(({ filename, locale, contentType }) => {
    const localeKey = Object.keys(LOCALE_IDS).find(
      (localeKey) => LOCALE_IDS[localeKey] === locale
    );
    return (
      !exclusions.excludePath[localeKey]?.some((path) =>
        filename.startsWith(path)
      ) && !exclusions.excludeType[localeKey]?.some((type) => contentType === type)
    );
  });
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh i misunderstood what you meant Kris, that does make complete and utter total sense

Copy link
Contributor

@aswanson-nr aswanson-nr left a comment

Choose a reason for hiding this comment

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

Looks great overall! I just think I found one small bug!

Copy link
Contributor

@aswanson-nr aswanson-nr left a comment

Choose a reason for hiding this comment

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

Overall looks great! Just noticed one thing in the exclusions file

@rudouglas rudouglas merged commit 3c786a3 into feature/machine-translation Dec 1, 2021
@rudouglas rudouglas deleted the ru/exclude-file branch December 1, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eng issues related to site functionality that requires engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Machine Translation] Create method for excluding specific directories/types
4 participants