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

Strings #862

Open
wants to merge 7 commits into
base: development
Choose a base branch
from
Open

Strings #862

wants to merge 7 commits into from

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Oct 15, 2023

Resolves #837

Quality Assurance: test case written

Just came to me while sorting types for another piece of work

@netlify
Copy link

netlify bot commented Oct 15, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 0a83bf7
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/65426105a9b711000814be38
😎 Deploy Preview https://deploy-preview-862--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 15, 2023

idk why I've got 3 week old commits in this, my dev branch is up to date with upstream and only made this branch this afternoon, fixed now won't happen again.

seems that CI is failing because of '.test.ts' but is needed for jest, I can remove the test or I'm open to ideas

rename test file
@Keyrxng Keyrxng marked this pull request as ready for review October 15, 2023 13:14
@0x4007
Copy link
Member

0x4007 commented Oct 17, 2023

This is interesting but I've never heard of f strings. Perhaps you can share some of your favorite resources on it and explain why you were inspired to take this approach?

It does look awfully pythonic which I have mixed feelings on. Perhaps there is a TypeScript-esqe parallel that we can draw inspiration from?

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Oct 17, 2023

The inspiration comes from using it with gpt prompts when building with langchain, it's readable and robust. The changes from Lang to this was just streamlining what we needed and optimizing things a little.

I don't think there is a TS parallel to draw from no at least not to my knowledge, it's a 'formatted string literal' similar to JS/TS \`${}`\ but from what I read we cannot use that approach and need to use something similar to Python's % string formatting.

I tried to find what you meant when you said this but I couldn't so I don't understand the issue with just using ${} so I figured this was a more elegant way to do things and it can be repurposed as across the bot as opposed to just the E2E tests

I presume the issue with using ${} is due to html formatting, if this is the reason then .replace() is the only built-in option and then we'd just replace HTML entities? I don't like the sound of it personally but if it's the preference then this could be refactored but I think this is superior

@Keyrxng Keyrxng mentioned this pull request Oct 21, 2023
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Nov 1, 2023

It just came to me that there is ofc a way to handle this with just TS and this is probably the sleekest also no longer caring about trying to escape although if using it for prompt template formatting then we should just use different delimiters <> as opposed to {} when prompting AI

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.

Tests: Share Strings
2 participants