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

Added config file for CLI #282

Merged
merged 10 commits into from
Jan 10, 2025
Merged

Added config file for CLI #282

merged 10 commits into from
Jan 10, 2025

Conversation

sifnoc
Copy link
Collaborator

@sifnoc sifnoc commented Dec 20, 2024

Related issue #264

  • Create Config.toml when mopro init
  • Use it mopro build and mopro create

The template selection is more informative like one below:

✔ Create template · ios          - Require bindings
Cannot create ios template - build binding first
? Create template ›
  ios          - Require bindings
  android
  web          - Require bindings
  flutter      - Requires ios bindings
  react-native - Requires ios bindings

@sifnoc sifnoc requested a review from vivianjeng December 20, 2024 19:52
cli/src/build.rs Outdated Show resolved Hide resolved
cli/src/create.rs Outdated Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Jan 7, 2025

Deploying mopro with  Cloudflare Pages  Cloudflare Pages

Latest commit: 02abf6a
Status: ✅  Deploy successful!
Preview URL: https://b6cced98.mopro.pages.dev
Branch Preview URL: https://cli-config.mopro.pages.dev

View logs

@sifnoc
Copy link
Collaborator Author

sifnoc commented Jan 7, 2025

I assumed that users will not manually remove the *Bindings folder in the initialized project created with mopro init.
Therefore, once a selected platform is written to the config file, it will not be removed.

@sifnoc sifnoc requested a review from vivianjeng January 7, 2025 09:25
Copy link
Collaborator

@vivianjeng vivianjeng left a comment

Choose a reason for hiding this comment

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

Kimi changes the term platform to framework
so I think maybe you need to merge the main branch to see the latest changes 🙏🏻

Comment on lines 120 to 136
// Adding more information on the list
let requires = ["ios", "android"];
let missing: Vec<&str> = requires
.iter()
.filter(|&&req| !config.target_platforms.contains(req))
.cloned()
.collect();

if !missing.is_empty() {
items.push(format!(
"{:<12} - Requires {} binding(s)",
template,
missing.join("/")
));
unselectable.push(true);
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I created the two bindings separately (ios and android)
and the Config.toml is updated correctly
but the mopro create for react-native and flutter still shows Require binding
Please check this

截圖 2025-01-09 上午11 41 39

Comment on lines 135 to 145
continue;
}
}

if config.target_platforms.contains(template) {
items.push(template.to_string());
unselectable.push(false);
} else {
items.push(format!("{:<12} - Require binding", template));
unselectable.push(true);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found the correct logic is like this
You should display the result of react-native and flutter if it is selectable

Suggested change
continue;
}
}
if config.target_platforms.contains(template) {
items.push(template.to_string());
unselectable.push(false);
} else {
items.push(format!("{:<12} - Require binding", template));
unselectable.push(true);
}
// continue;
} else {
items.push(template.to_string());
unselectable.push(false);
}
} else if config.target_platforms.contains(template) {
items.push(template.to_string());
unselectable.push(false);
} else {
items.push(format!("{:<12} - Require binding", template));
unselectable.push(true);
}

@sifnoc
Copy link
Collaborator Author

sifnoc commented Jan 9, 2025

Kimi changes the term platform to framework so I think maybe you need to merge the main branch to see the latest changes 🙏🏻

I believe the term 'framework' doesn't fit well in the 'build' phase. We've been using the term 'template' in the 'create' phase, so I decided to keep 'template' for printout strings in the 'create' phase while using 'framework' in the code for consistency.

Copy link
Collaborator

@vivianjeng vivianjeng 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!

@vivianjeng vivianjeng merged commit dfb9b28 into main Jan 10, 2025
35 checks passed
@vivianjeng vivianjeng deleted the cli-config branch January 10, 2025 06:32
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.

2 participants