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

Update Functions.swift #45

Closed
wants to merge 10 commits into from
Closed

Update Functions.swift #45

wants to merge 10 commits into from

Conversation

jaysonng
Copy link
Contributor

Fix buildServerURL to not include default port 8081 when running in localhost or 127.0.0.1

Fix buildServerURL to check for default port 8081
@cbaker6
Copy link
Member

cbaker6 commented Jul 13, 2023

These changes need to be linted (see the CI log, you can lint from the terminal).

Also, the port shouldn’t be decided like this. If there’s no port, you can make it not specify a default and the forcing to local host shouldn’t be there either. The changes look particular to your usecase, and not universal for most usecases.

@jaysonng
Copy link
Contributor Author

Hi,

In ParseServerConfiguration the port 8081 is set as default and I didn't think touching all of that was way to go.

How do I " If there’s no port, you can make it not specify a default" ? as 8081 is set as the port by this point if there's no port set in ENV.

What do you think is best way to achieve this? I'll be happy to fix it to work better in a general sense.

@cbaker6 cbaker6 marked this pull request as draft September 24, 2023 12:04
@jaysonng
Copy link
Contributor Author

Would love to hear your thoughts on this and maybe get it a fix in pulled.

@cbaker6
Copy link
Member

cbaker6 commented Jan 13, 2024

You have merged changes from your other branches into here, typically that can create merge conflicts. It doesn’t look the comments have been addressed from #45 (comment)

@cbaker6
Copy link
Member

cbaker6 commented Jan 14, 2024

Closing this, feel free to open another PR if you have a different fix.

@cbaker6 cbaker6 closed this Jan 14, 2024
@jaysonng
Copy link
Contributor Author

thanks for the fix!

@jaysonng jaysonng deleted the main branch January 15, 2024 04:43
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