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

Add an api that gives higher priority options #34

Merged
merged 4 commits into from
Nov 13, 2023

Conversation

abcd-ts
Copy link
Contributor

@abcd-ts abcd-ts commented Oct 30, 2023

close #27

概要

設定ファイルより優先したいオプションを記述した JSON 文字列を与えることができる API を追加しました。

実装

JSON文字列とパスを与えて、JSON に書かれているオプションを優先するような Config 構造体を生成する
Config::from_settings_json_and_config_path メソッドを実装しています。

その他

メソッドやAPIの命名について、他に良い案があれば教えていただきたいです。

@tanzaku
Copy link
Collaborator

tanzaku commented Nov 1, 2023

メソッドやAPIの命名について、他に良い案があれば教えていただきたいです

命名とは少しずれますが、json文字列の有無 x 設定ファイルの有無で4種類の関連関数を作ることになってしまっているので、以下のようにnewの引数でjson文字列と設定ファイルをOptionで受け取るようにする案はどうでしょうか。

newの引数でjson文字列と設定ファイルをOptionで受け取る案
    pub fn new(
        settings_json: Option<&str>,
        path: Option<&str>,
    ) -> Result<Config, UroboroSQLFmtError> {
        let mut config = serde_json::Map::new();

        // 設定ファイルから読み込む
        if let Some(path) = path {
            let file = File::open(path).map_err(|_| {
                UroboroSQLFmtError::FileNotFound("Setting file not found".to_string())
            })?;

            let reader = BufReader::new(file);

            let file_config: serde_json::Map<String, serde_json::Value> =
                serde_json::from_reader(reader)
                    .map_err(|e| UroboroSQLFmtError::IllegalSettingFile(e.to_string()))?;

            for (key, value) in file_config {
                config.insert(key, value);
            }
        }

        // json文字列から読み込む
        if let Some(settings_json) = settings_json {
            let settings: serde_json::Map<String, serde_json::Value> =
                serde_json::from_str(settings_json).map_err(|e| {
                    UroboroSQLFmtError::Runtime(format!(
                        "Setting json is invalid.{}",
                        &e.to_string()
                    ))
                })?;

            for (key, value) in settings {
                config.insert(key, value);
            }
        }

        serde_json::from_value(serde_json::Value::Object(config))
            .map_err(|e| UroboroSQLFmtError::Runtime(e.to_string()))
    }

従来のnewで作成していたデフォルト値を持つインスタンスはDefault traitで作成する想定です。

Default
impl Default for Config {
    fn default() -> Self {
        Config {
            debug: default_debug(),
            tab_size: default_tab_size(),
            complement_alias: default_complement_alias(),
            trim_bind_param: default_trim_bind_param(),
            keyword_case: Case::default(),
            identifier_case: Case::default(),
            max_char_per_line: default_max_char_per_line(),
            complement_outer_keyword: default_complement_outer_keyword(),
            complement_column_as_keyword: default_complement_column_as_keyword(),
            remove_table_as_keyword: default_remove_table_as_keyword(),
            remove_redundant_nest: default_remove_redundant_nest(),
            complement_sql_id: default_complement_sql_id(),
            convert_double_colon_cast: default_convert_double_colon_cast(),
            unify_not_equal: default_unify_not_equal(),
        }
    }
}

@abcd-ts
Copy link
Contributor Author

abcd-ts commented Nov 2, 2023

命名とは少しずれますが、json文字列の有無 x 設定ファイルの有無で4種類の関連関数を作ることになってしまっているので、以下のようにnewの引数でjson文字列と設定ファイルをOptionで受け取るようにする案はどうでしょうか。

ありがとうございます。確かに異なる4種類の関数は必要ないので、示していただいたように new でConfig を生成するように変更しました。

似ている問題で、uroborosql-fmt の公開関数 format_sql (従来のままで、ソース文字列と設定ファイルのパスのみを受け取る)と今回追加した format_sql_with_settings_json も、json文字列を Option に変更することで統合できるのですが、そのようにしてしまって良いでしょうか?
公開しているので念の為変更していませんでした。

変更後

pub fn format_sql(
    src: &str,
    settings_json: Option<&str>,
    config_path: Option<&str>,
) -> Result<String, UroboroSQLFmtError> { /**/ }

なお、uroborosql-fmt-napi の公開関数 runfmtrunfmt_with_settings は、runfmt をvscode拡張で参照しているので、現状は変更しないつもりです。

@tanzaku
Copy link
Collaborator

tanzaku commented Nov 8, 2023

似ている問題で、uroborosql-fmt の公開関数 format_sql (従来のままで、ソース文字列と設定ファイルのパスのみを受け取る)と今回追加した format_sql_with_settings_json も、json文字列を Option に変更することで統合できるのですが、そのようにしてしまって良いでしょうか?

いいですね!
一般論としては公開関数の変更をマイナーバージョンアップで行うのは望ましくないとは思いますが、crates.ioにまだ上げておらず統合しても問題はほぼ起きないと思うので良いと思います。

@abcd-ts
Copy link
Contributor Author

abcd-ts commented Nov 9, 2023

ありがとうございます。統合しました!

@tanzaku
Copy link
Collaborator

tanzaku commented Nov 13, 2023

LGMT

@tanzaku tanzaku merged commit ee9c086 into main Nov 13, 2023
8 checks passed
@tanzaku tanzaku deleted the saito-add-api-for-vscode branch November 13, 2023 01:57
@tanzaku
Copy link
Collaborator

tanzaku commented Nov 13, 2023

conflictしていたのでrebase実施してマージしました。

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.

Add an API that allows options to be passed in a settings file and json string
2 participants