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

Generate port settings in ports #11110

Closed
wants to merge 3 commits into from
Closed

Generate port settings in ports #11110

wants to merge 3 commits into from

Conversation

kripken
Copy link
Member

@kripken kripken commented May 8, 2020

Instead of a port having to modify settings.js, give them a hook to
add new settings.

See discussion in #11066

@kripken kripken requested a review from sbc100 May 8, 2020 20:19
@sbc100
Copy link
Collaborator

sbc100 commented May 9, 2020

Hmm.. I think I like this approach. I was going to suggest an alternative approach, which I think maybe we should still take in addition to this.

We have a single setting called USE_PORT=foo which triggers the loading and activation of a port called foo.py. This would mean that foo.py wouldn't even get parse or loaded unless that user ask for it and ports would not need to register their own settings (making each port slightly simpler and ensuring consistency).

I think either way this change you have here is a nice simplification and cleanup up settings.js quite nicely.

@kripken
Copy link
Member Author

kripken commented May 11, 2020

I don't feel strongly between the approaches - your approach may be a simpler change actually.

@sbc100
Copy link
Collaborator

sbc100 commented May 11, 2020

I think we could do both maybe? This approach for the current set.. and the USE_PORT for any new ones we add?

@sbc100
Copy link
Collaborator

sbc100 commented May 11, 2020

Actually I think I do like my idea better and maybe this PR just makes this more complicated for the existing ports?

@kripken
Copy link
Member Author

kripken commented May 18, 2020

@sbc100 your idea sounds better to me too, yeah, it's nice to avoid having to do any work for a port unless it's actually used.

I'll close this then.

@kripken kripken closed this May 18, 2020
@kripken kripken deleted the autoport branch May 18, 2020 23:34
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