You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "trim21 (via GitHub)" <gi...@apache.org> on 2024/12/25 17:20:18 UTC
[PR] dev: generate python types [opendal]
trim21 opened a new pull request, #5457:
URL: https://github.com/apache/opendal/pull/5457
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
This PR include 2 parts, a python generate and parser improvement.
Parser not will take `bool` type as optional and parse deprecated note from config attr.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking-changes` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898185502
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
Also https://github.com/rinja-rs/askama/issues/1035
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898164871
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I'm not sure if this is a good idea... I tried type-safe template engine https://github.com/rinja-rs/askama and result into rewrite `Services` again to get some field sorted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898214087
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
Then I think rinja is a acceptable option here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898235410
##########
bindings/python/python/opendal/__base.pyi:
##########
Review Comment:
Hi, cc @trim21 @messense what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1897696665
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I'm thinking of using https://crates.io/crates/handlebars instead of building string directly.
##########
bindings/python/python/opendal/__base.pyi:
##########
Review Comment:
So, we just generate the `.pyi` code here. Would it work better if we had classes or types like `S3Config` on the Python side? This way, we could include more information, such as comments.
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
Review Comment:
Perhaps we could embed the templates here, so we won't need to edit multiple files whenever we want to modify the Python code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898027402
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
Review Comment:
I suggest we do not embed it in rust code, seprated python file can have syntax highlight, auto completions and can be formatted by ruff(python formatter we used)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898214087
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
Then I think rinja is a acceptable option
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2563348998
I guess rust code is fine for now? Still need to resolve what kind of typing we want to generate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898184019
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
askama seems cool but it's latest release is one year ago, I have a bit concern about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2562967247
> For me, the PR mostly LGTM, but some idea
>
> 1. We need to think more about `_int` type, a str type but with integer semantic
> 2. If we can drop support for the version under 3.12, we may can use typedict for kwargs, FYI https://peps.python.org/pep-0692/
We do not need to drop py<3.12 to use this feature, pyi file is not runtime code, as long as the type checker support pep692.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898271080
##########
bindings/python/python/opendal/__base.pyi:
##########
Review Comment:
we may need to rewrite some rust code to make it work with pyo3-stub-gen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898235180
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
Cool, let's do this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898187024
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I'd use https://github.com/mitsuhiko/minijinja (but IMO any one of them is fine choice as long as the templating crate works and is actively maintained).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898236192
##########
bindings/python/python/opendal/__base.pyi:
##########
Review Comment:
The other concern here is that I would prefer not to maintain `.pyi` files ourselves in the future. We should use tools like a stub file generator, as mentioned in https://github.com/PyO3/pyo3/issues/510, such as https://crates.io/crates/pyo3-stub-gen (raising by @trim21).
Instead, we can simply keep the struct in Python with comments and let other tools generate the `.pyi` files for us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898276333
##########
dev/src/generate/binding_python.rs:
##########
@@ -70,11 +70,11 @@ pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
fs::write(output_file.clone(), t).expect("failed to write result to file");
- Command::new("ruff")
- .arg("format")
- .arg(output_file)
- .output()
- .expect("failed to run ruff");
+ let ruff = Command::new("ruff").arg("format").arg(output_file).output();
Review Comment:
Please don't call `ruff` in `odev`. We can do those job in `justfile`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2563357348
> I thought we still have some opinion on `_int` `_duration` type alias?
It's not a blocking issue, this improves the status quo of typing for python binding already. The rest can be followed up later. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898185816
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
we can use rinja maybe？ it's the template engine docs.rs is using. what we need to switch is to change crate name from `askama` to `rinja`, this PR doesn't use any filter so it's easy to do that.
There is also a factor that I'm a python developer so I'm familiar with template langauge jinja.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898185816
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
we can use rinja maybe？ it's the template engine docs.rs is using. what we need to switch is to change crate name from `askama` to `rinja`, this PR doesn't use any filter so it's easy to do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] dev: generate python types [opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1897520669
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,86 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+fn enabled_service(srv: &str) -> bool {
Review Comment:
Sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] dev: generate python types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2562082706
> Mostly LGTM, but I think we should revist the `_bool` and `_int` type aliases (due to the use of `Operator::from_iter` that only accepts a iterator of `(String, String)`), IMHO at least the `_int` type should accepts `int`.
pyo3 doesn't do implicit type conversion
```text
Traceback (most recent call last):
File "C:\Users\Trim21\proj\test\f.py", line 4, in <module>
opendal.Operator("redis", db=1)
pyo3_runtime.PanicException: must be valid hashmap: PyErr { type: <class 'TypeError'>, value: TypeError("'int' object cannot be converted to 'PyString'"), traceback: None }
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898027907
##########
dev/src/generate/parser.rs:
##########
@@ -53,6 +53,8 @@ pub struct Config {
pub value: ConfigType,
/// If given config is optional or not.
pub optional: bool,
+ /// if this field is deprecated, a deprecated message will be provided.
+ pub deprecated: Option<String>,
Review Comment:
https://github.com/apache/opendal/pull/5463
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898164871
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I'm not sure if this is a good idea... I tried type-safe template engine https://github.com/rinja-rs/askama
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898164871
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I choose https://github.com/rinja-rs/askama, looks like more easy to use handlebars
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898027402
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
Review Comment:
I suggest we do not embed it in rust code, it's hard to edit other langauge in a long rust string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1897751420
##########
dev/src/generate/parser.rs:
##########
@@ -53,6 +53,8 @@ pub struct Config {
pub value: ConfigType,
/// If given config is optional or not.
pub optional: bool,
+ /// if this field is deprecated, a deprecated message will be provided.
+ pub deprecated: Option<String>,
Review Comment:
Hi, would you like to split those logic in a seperate PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5457:
URL: https://github.com/apache/opendal/pull/5457
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] dev: generate python types [opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1897516412
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,86 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+fn enabled_service(srv: &str) -> bool {
Review Comment:
Consider use `cargo_metadata` to get the enabled feature list? https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.Node.html#structfield.features
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898185816
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
we can use rinja maybe？ it's the template engine docs.rs is using. what we need to switch is to change crate name from `askama` to `rinja`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898186548
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
https://blog.guillaume-gomez.fr/articles/2024-07-31+docs.rs+switching+jinja+template+framework+from+tera+to+rinja
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2562570892
For me, the PR mostly LGTM, but some idea
1. We need to think more about `_int` type, a str type but with integer semantic
2. If we can drop support for the version under 3.12, we may can use typedict for kwargs, FYI https://peps.python.org/pep-0692/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898027402
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
Review Comment:
I suggest we do not embed it in rust code, seprated python can have syntax highlight and can be formatted by ruff(python formatter we used)
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
Review Comment:
I suggest we do not embed it in rust code, seprated python file can have syntax highlight and can be formatted by ruff(python formatter we used)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898185816
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
we can use rinja maybe？ it's the template engine docs.rs is using. what we need to switch is to change crate name from `askama` to `rinja`, this PR doesn't use any filter so it's easy to do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898238193
##########
bindings/python/python/opendal/__base.pyi:
##########
Review Comment:
> Would it work better if we had classes or types like `S3Config` on the Python side? This way, we could include more information, such as comments.
I think having Python config types is better, but it can be done later.
Use pyo3-stub-gen would solve typing issue in python side (assuming it can satisfy our needs, I've never tried it so I'm not sure), not sure about other bindings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] dev: generate python types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2562059632
> Mostly LGTM, but I think we should revist the `_bool` and `_int` type aliases (due to the use of `Operator::from_iter` that only accepts a iterator of `(String, String)`), IMHO at least the `_int` type should accepts `int`.
it should be some kind of `HashMap<String, Enum(String,uint,int,bool)>` I guess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2562228155
> `FsConfig.root` is `Option<String>` which is actually required.
I think we can fix it from the core side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898164871
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I choose https://github.com/rinja-rs/askama, looks like more easy to use than handlebars and get compile time checking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898164871
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I'm not sure if this is a good idea... I'll take a look onhttps://github.com/rinja-rs/askama
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
I'm not sure if this is a good idea... I'll take a look on https://github.com/rinja-rs/askama
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] dev: generate python types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1897517989
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,86 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+fn enabled_service(srv: &str) -> bool {
Review Comment:
Sounds like a good idea, but I'm not very enthusiasm in implement this, it's it ok I add a TODO on it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898186548
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
+ .expect("failed to read python template file");
+
+ for (srv, config) in services.clone().into_iter() {
+ if !enabled_service(srv.as_str()) {
+ continue;
+ }
+
+ s.push_str("\n @overload\n");
Review Comment:
https://github.com/rust-lang/docs.rs/blob/16ebd6f394a04c8428f7df63fdf59fd8c37eb9ed/Cargo.toml#L92
https://blog.guillaume-gomez.fr/articles/2024-07-31+docs.rs+switching+jinja+template+framework+from+tera+to+rinja
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5457:
URL: https://github.com/apache/opendal/pull/5457#discussion_r1898238364
##########
dev/src/generate/binding_python.rs:
##########
@@ -17,9 +17,87 @@
use crate::generate::parser::Services;
use anyhow::Result;
+use itertools::Itertools;
+use std::fs;
+use std::path::PathBuf;
-pub fn generate(services: &Services) -> Result<()> {
- println!("{:?}", services);
+use super::parser::ConfigType;
+
+/// TODO: add a common utils to parse enabled features from cargo.toml
+fn enabled_service(srv: &str) -> bool {
+ match srv {
+ // not enabled in bindings/python/Cargo.toml
+ "etcd" | "foundationdb" | "ftp" | "hdfs" | "rocksdb" | "tikv" => false,
+ _ => true,
+ }
+}
+
+pub fn generate(project_root: PathBuf, services: &Services) -> Result<()> {
+ let mut s = fs::read_to_string(project_root.join("dev/templates/python"))
Review Comment:
The other perspective also makes sense to me: moving all string construction logic into a template file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "trim21 (via GitHub)" <gi...@apache.org>.
trim21 commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2563354136
> Thank you @trim21 for working this, really great! Also thanks @messense and @Zheaoli's review. Let's move on!
I thought we still have some opinion on `_int` `_duration` typg alias?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/python): generate python operator constructor types [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5457:
URL: https://github.com/apache/opendal/pull/5457#issuecomment-2563354065
Hi, @trim21 this will be large task, would you like to create a tracking issue for the progress?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org