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

chore: allow users context traits and underscore divide numbers configuration #3703

Closed
wants to merge 7 commits into from

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Sep 3, 2024

What are the changes introduced in this PR?

  • Playtomic feedback:
    • allowUsersContextTraits when set to false, if context.traits.* is present, it will be added only as context_traits_*.
    • underscoreDivideNumbers when set to false, if a column has a format like "*v3*", it will be formatted to "*v3*".
  • We need to populate those fields so that the older destinations behave similarly.

What is the related Linear task?

  • Resolves PIPE-1262.
  • Resolves PIPE-1413.

Developer checklist

  • My code follows the style guidelines of this project
  • No breaking changes are being introduced.
  • All related docs linked with the PR?
  • All changes manually tested?
  • Any documentation changes needed with this change?
  • Is the PR limited to 10 file changes?
  • Is the PR limited to one linear task?
  • Are relevant unit and component test-cases added in new readability format?

Reviewer checklist

  • Is the type of change in the PR title appropriate as per the changes?
  • Verified that there are no credentials or confidential data exposed with the changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not being used since we set the default version to v1.

@devops-github-rudderstack
Copy link
Contributor

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.86%. Comparing base (d70ac5d) to head (4e1eb91).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3703      +/-   ##
===========================================
+ Coverage    88.69%   88.86%   +0.16%     
===========================================
  Files          594      595       +1     
  Lines        32413    32425      +12     
  Branches      7727     7711      -16     
===========================================
+ Hits         28750    28815      +65     
+ Misses        3342     3333       -9     
+ Partials       321      277      -44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@achettyiitr achettyiitr force-pushed the chore.playtomic-feedback branch 2 times, most recently from d21ec7b to 85f7f2d Compare September 5, 2024 10:07
@achettyiitr achettyiitr added the DO NOT MERGE Don't merge this PR without having discussion label Sep 5, 2024
src/warehouse/v1/util.js Outdated Show resolved Hide resolved
@achettyiitr achettyiitr force-pushed the chore.playtomic-feedback branch 3 times, most recently from 25ba755 to 77673c2 Compare September 10, 2024 06:52
Copy link
Member Author

Choose a reason for hiding this comment

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

The following implementation has been take from here and here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The following implementation has been take from here.

Copy link
Member Author

Choose a reason for hiding this comment

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

With the additional change for reUnicodeWordsWithNumbers which has two additional regexes:

  1. ${rsDigit}+${rsUpper}?${rsLower}+, // Digits followed by lowercase letters (e.g., "123abc")
  2. ${rsDigit}+${rsUpper}+, // Digits followed by uppercase letters (e.g., "123ABC")
const reUnicodeWordsWithNumbers = RegExp(
  [
    `${rsUpper}?${rsLower}+${rsDigit}+`, // Lowercase letters followed by digits (e.g., "abc123")
    `${rsUpper}+${rsDigit}+`, // Uppercase letters followed by digits (e.g., "ABC123")
    `${rsDigit}+${rsUpper}?${rsLower}+`, // Digits followed by lowercase letters (e.g., "123abc")
    `${rsDigit}+${rsUpper}+`, // Digits followed by uppercase letters (e.g., "123ABC")
    `${rsUpper}?${rsLower}+${rsOptContrLower}(?=${[rsBreak, rsUpper, '$'].join('|')})`, // Regular words, lowercase letters followed by optional contractions
    `${rsMiscUpper}+${rsOptContrUpper}(?=${[rsBreak, rsUpper + rsMiscLower, '$'].join('|')})`, // Miscellaneous uppercase characters with optional contractions
    `${rsUpper}?${rsMiscLower}+${rsOptContrLower}`, // Miscellaneous lowercase sequences with optional contractions
    `${rsUpper}+${rsOptContrUpper}`, // All uppercase words with optional contractions (e.g., "THIS")
    rsOrdUpper, // Ordinals for uppercase (e.g., "1ST", "2ND")
    rsOrdLower, // Ordinals for lowercase (e.g., "1st", "2nd")
    `${rsDigit}+`, // Pure digits (e.g., "123")
    rsEmoji, // Emojis (e.g., 😀, ❤️)
  ].join('|'),
  'g',
);

cisse21
cisse21 previously approved these changes Sep 13, 2024
cisse21
cisse21 previously approved these changes Sep 15, 2024
BonapartePC
BonapartePC previously approved these changes Sep 18, 2024
@achettyiitr achettyiitr changed the title chore: playtomic feedback chore: allow users context traits and underscore divide numbers configuration Sep 19, 2024
@achettyiitr achettyiitr removed the DO NOT MERGE Don't merge this PR without having discussion label Sep 23, 2024
Copy link

sonarcloud bot commented Sep 23, 2024

@achettyiitr achettyiitr deleted the chore.playtomic-feedback branch September 23, 2024 14:17
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.

5 participants