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

Move Config read/write into cli #109

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Move Config read/write into cli #109

merged 1 commit into from
Feb 24, 2020

Conversation

NotWoods
Copy link
Contributor

Moves functions for reading/writing Config into the cli, since none of them are used in core. check() has been removed since its no longer used.

Also added a path parameter so a prompt could be added to allow custom config paths.

Core Config is now just a data class, so perhaps it should be changed to an interface.

@PEConn
Copy link
Collaborator

PEConn commented Feb 24, 2020

I'm happy with adding the path, unsure about whether methods for reading/writing the config should be part of the core library even if they're not used there. @andreban thoughts?

@andreban
Copy link
Member

Functions to load and save the config would ideally be part of the library.

The section of code that tries to load a config from a file and asks users for config information if it fails (loadOrCreateConfig) and createConfig should be part of the CLI.

We may also want to add Config#serialize(): string and Config#deserialise(config: string): Config to make it easier for developers who'd like to save a Config to a database, for instance.

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

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

We should move save and load methods into core.

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

@andreban andreban merged commit 43c2e2d into GoogleChromeLabs:master Feb 24, 2020
@andreban andreban added enhancement New feature or request breaking internal and removed enhancement New feature or request labels Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants