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.

Sign up for GitHub

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

Jump to bottom

Disallow some empty config URL settings #412

Merged
merged 3 commits into from
Jul 13, 2022
Merged

Disallow some empty config URL settings #412

merged 3 commits into from
Jul 13, 2022

Conversation

AndrewFerr
Copy link
Member

If these settings are meant to be unspecified, then their entire parent
sections (widgets & generic) should be unspecified.

Signed-off-by: Andrew Ferrazzutti andrewf@element.io

@AndrewFerr
Disallow some empty config URL settings
7dfc078 
If these settings are meant to be unspecified, then their entire parent
sections (`widgets` & `generic`) should be unspecified.

Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
@AndrewFerr AndrewFerr requested a review from Half-Shot July 12, 2022 16:26
Half-Shot
Half-Shot reviewed Jul 12, 2022
View reviewed changes
src/Config/Config.ts Outdated Show resolved Hide resolved
src/Config/Config.ts Outdated Show resolved Hide resolved
@AndrewFerr
Convert some config URL strings to URL objects
a02e64c 
This allows both parsing and easier crafting of relative URLs.

Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
@AndrewFerr
Dump parsed URLs to default config
4bfc0f0 
Also implement getters to return stringified URLs, instead of having to
store a URL's string representation directly.

Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
@AndrewFerr
Copy link
Member Author

The URLs are now parsed, and also stored as URLs (not just strings) in the config objects.

4bfc0f0 goes maybe a bit too far in allowing using URL config values for the dumped sample config file, and without needing to store a stringified copy of each URL. Using dynamically assigned functions (instead of getters) was needed for the dumper to notice them & dump them in the proper order.

Half-Shot
Half-Shot reviewed Jul 13, 2022
View reviewed changes
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

This looks fine, just the url concat behaviour needs looking at.

src/Connections/GenericHook.ts Show resolved Hide resolved
Half-Shot
Half-Shot approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

I bow to your greater understanding of URL :)

@AndrewFerr AndrewFerr merged commit d4f701c into main Jul 13, 2022
@AndrewFerr AndrewFerr deleted the af/empty-url-configs branch July 13, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Half-Shot Half-Shot Half-Shot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AndrewFerr @Half-Shot