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

Move bulk of title formatting logic to separate library #242

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

netux
Copy link

@netux netux commented Apr 10, 2024

  • Bump maze-utils to commit 02978e01, which contains the moved title formatter logic

  • Uninstall cld3-asm, as all code using it is now in maze-utils

  • Change imports throughout to use types and methods from maze-utils

  • Delete title formatter tests. These now live in maze-utils

  • I agree to license my contribution under GPL-3.0 and agree to allow distribution on app stores as outlined in LICENSE-APPSTORE

To test this pull request, follow the instructions in the wiki.


Second half of #241.

Footnotes

  1. I'm not 100% sure how cross-gitmodule changes should work. In theory, if the maze-utils submodule is pinned to a specific commit then this should be fine, right?

- Bump maze-utils to commit 02978e0, which contains the moved title formatter logic
- Uninstall cld3-asm, as all code using it is now in maze-utils
- Change imports throughout
- Delete title formatter tests. These now live in maze-utils
@ajayyy
Copy link
Owner

ajayyy commented Apr 10, 2024

At the moment maze utils is not imported as a module, but a folder, so I'm not sure it would work to delete the cld-3 dependency. I think it won't compile

@ajayyy
Copy link
Owner

ajayyy commented Apr 10, 2024

Your understanding of submodules is correct

@netux
Copy link
Author

netux commented Apr 10, 2024

At the moment maze utils is not imported as a module, but a folder, so I'm not sure it would work to delete the cld-3 dependency. I think it won't compile

Hmm, it was working on my machine. I guess I'll add that dependency back then.

@netux
Copy link
Author

netux commented Apr 11, 2024

Hmm, it was working on my machine. I guess I'll add that dependency back then.

I'm now looking for a way to make the dependency optional. I was thinking putting it in peer dependencies of maze-utils and wrapping the import in a try...catch so SponsorBlock could compile without it, but...

  1. Webpack complains about the import when cld3-asm is not installed, even when LOAD_CLD = false
  2. Even if that's somehow solved, there is yet another import of cld3-asm, but only for one of its types. I doubt there is any way to conditionally exclude type imports in Webpack

So I'll now look into moving all of the title formatting stuff into its own library.
@ajayyy could you create a repo for this? It makes more sense if its under your name and control.

@ajayyy
Copy link
Owner

ajayyy commented Apr 11, 2024

Created a repo: https://github.com/ajayyy/DeArrowFormatting

@netux netux changed the title Move bulk of title formatting logic to maze-utils Move bulk of title formatting logic to separate library Apr 12, 2024
@netux
Copy link
Author

netux commented May 8, 2024

Hello. Any hopes of this getting looked at anytime soon? 😄

@@ -19,7 +19,7 @@ export const FormattedText = (props: FormattedTextProps) => {
(async () => {
const text = "text" in props ? props.text : chrome.i18n.getMessage(props.langKey);

setLabel(await formatTitleInternal(text, false, props.titleFormatting!, false));
setLabel(await formatTitle(text, false, props.titleFormatting!, false, Config.config!.onlyTitleCaseInEnglish));
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason for this change?

Copy link
Author

Choose a reason for hiding this comment

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

There are two changes here:

  1. Replace formatTitleInternalformatTitle: note that formatTitle comes from DeArrowFormatting, not from titles/titleFormatter.ts. I just moved formatTitleInternal in DeArrowFormatting, and renamed it to formatTitle.
    • As to why I changed the name: I thought it didn't make sense to label the function "internal" when, from the PoV of DeArrowFormatting, it was the function being exposed to the outside.
      From the PoV of DeArrow, the function is used internally (which is part of the reason why I aliased it to formatTitleInternal in src/titles/titleFormatter.ts)
  2. Added Config.config!.onlyTitleCaseInEnglish parameter: DeArrowFormatting doesn't know about DeArrow's Config object anymore, so we have to pass it manually.

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