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

Rename schema fields and add a jsonschema generation and ref on init #361

Merged
merged 9 commits into from
Mar 13, 2024

Conversation

soupi
Copy link
Contributor

@soupi soupi commented Mar 11, 2024

What

This PR does the following:

  1. It renames a few fields in the configuration:
    • configureOptions -> introspectionOptions
    • mutationsVersion moved from configureOptions to the top-level
    • connection uri, pool settings and isolation level are now nested under "connectionSettings"
    • Native queries use a new ReadonlyColumnInfo type instead of ColumnInfo type for arguments and column. ReadonlyColumnInfo does not contain default, identity and generated information which we don't want to appear in autocomplete for native queries.
  2. It generates a schema.jsonschema file on cli initialize command next to the configuration.json file
  3. It adds the $schema field to the configuration, referencing the generated schema.jsonschema file.

The purpose of (2) and (3) is to provide auto-complete features when editing the configuration in vscode (demo soon).

Screenshot from 2024-03-12 16-56-07

@soupi soupi changed the title add a schema command which prints the jsonschema of the connector config Rename schema fields and add a jsonschema generation and ref on init Mar 12, 2024
@soupi soupi requested review from SamirTalwar and plcplc March 12, 2024 15:02
justfile Show resolved Hide resolved
@@ -2813,7 +2816,7 @@
"operatorKind": "custom"
}
],
"mutationsVersion": null,
"mutationsVersion": "v1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should mutationsVersion change to "v1"?

Copy link
Contributor

@SamirTalwar SamirTalwar left a comment

Choose a reason for hiding this comment

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

I like this a lot.

@@ -14,6 +14,7 @@ use crate::values::{ConnectionUri, IsolationLevel, PoolSettings, Secret};
use crate::version3;

pub const CONFIGURATION_FILENAME: &str = "configuration.json";
pub const CONFIGURATION_JSONSCHEMA_FILENAME: &str = "schema.jsonschema";
Copy link
Contributor

Choose a reason for hiding this comment

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

Did some reading and apparently schema.json and blahblah.schema.json are more common.

Suggested change
pub const CONFIGURATION_JSONSCHEMA_FILENAME: &str = "schema.jsonschema";
pub const CONFIGURATION_JSONSCHEMA_FILENAME: &str = "schema.json";

Comment on lines -82 to +86
pool_settings: configuration.pool_settings,
pool_settings: configuration.connection_settings.pool_settings,
connection_uri,
isolation_level: configuration.isolation_level,
mutations_version: configuration.configure_options.mutations_version,
isolation_level: configuration.connection_settings.isolation_level,
mutations_version: configuration.mutations_version,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it's worth just storing connection_settings directly in the Configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about it, but didn't want to make too many changes, and wasn't sure about whether we should use version3 structure here. Maybe we can decide in a separate PR since this is internal to the connector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I do agree that that's a better structure though.

justfile Show resolved Hide resolved
Copy link
Contributor

@plcplc plcplc left a comment

Choose a reason for hiding this comment

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

Looks good to me.

With this PR the configuration fields are better grouped according to their usage.

IMO only metadata.nativeQueries stands out a bit, since it's authored by users while the rest of the metadata object is generated. Whether we should do anything about that or not is a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

What generates this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a manual copy paste tbh. Maybe we can decide later how to handle this for local dev?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the only purpose is to have something for the $schema field to reference I would be fine with just eliding the file

Copy link
Contributor

Choose a reason for hiding this comment

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

That is, the schema field in the test-configurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's exactly it's use.

@soupi soupi marked this pull request as ready for review March 13, 2024 10:21
@soupi soupi enabled auto-merge March 13, 2024 12:35
@soupi soupi added this pull request to the merge queue Mar 13, 2024
Merged via the queue into main with commit ebb7b53 Mar 13, 2024
33 of 35 checks passed
@soupi soupi deleted the gil/cli-jsonschema branch March 13, 2024 12:44
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.

3 participants