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

Replace String Class with conversion to String #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lesoup-mxd
Copy link

The earlier approach raised compile errors, and created an unnecessary object. This approach should solve both and make the return more uniformal

@lesoup-mxd
Copy link
Author

Compiles!
image

@Jensen-holm
Copy link
Owner

Jensen-holm commented Dec 3, 2024

Hi lesoup, thanks for your PR.

I don't fully understand the need for the string change, could you explain a bit more?

@Jensen-holm
Copy link
Owner

I just discovered this when looking at the latest change log from Mojo

image

I think that this would be a better fix overall. If you could implement this then I will merge the pr.

@lesoup-mxd
Copy link
Author

lesoup-mxd commented Dec 3, 2024 via email

@lesoup-mxd
Copy link
Author

Yea, seems better. Also, I see a couple variable declarations using the old "let" expr. that need to be updated

@lesoup-mxd
Copy link
Author

It's present on your blog, too, so it would be a good idea to fix asap. Also, I found a couple other backends instead of relying on python modules, which seems like a wise idea

@Jensen-holm
Copy link
Owner

I agree with all of the things that you're saying, however I don't really have the time or energy to implement these changes myself.

At the time I created this project Mojo was in a much earlier stage of development, and the way that I implemented this was the standard at the time. I don't plan to personally keep up with the changes of Mojo, since I have other projects that I want to spend my time on.

Personally, I no longer use FireTCP myself and it is more of a 'toy' project to me now. But if this is something that you use and would like to improve on, I can create issues for these improvements and assign them to you.

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