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

Feature/require script consistency #1451

Merged
merged 5 commits into from
May 27, 2022

Conversation

ProofOfKeags
Copy link
Contributor

after #1437

@ProofOfKeags ProofOfKeags requested a review from dr-bonez May 18, 2022 18:01
@ProofOfKeags ProofOfKeags force-pushed the feature/require-script-consistency branch from a7e768e to 93033e4 Compare May 19, 2022 19:43
Blu-J and others added 4 commits May 27, 2022 12:37
wip: Getting async js

feat: Have execute get action config

feat: Read + Write

chore: Add typing for globals

chore: Change the default path, include error on missing function, and add json File Read Write

chore: Change the default path, include error on missing function, and add json File Read Write

wip: Fix the unit test

wip: Fix the unit test

feat: module loading
@ProofOfKeags ProofOfKeags force-pushed the feature/require-script-consistency branch from 93033e4 to 238c1c6 Compare May 27, 2022 18:42
@ProofOfKeags ProofOfKeags requested a review from Blu-J May 27, 2022 18:59
@ProofOfKeags ProofOfKeags added Enhancement New feature or request P1 - Blocks Release This issue must be addressed before the coming release is shipped start-sdk Issues pertaining to the cli tool for packaging services labels May 27, 2022
@ProofOfKeags ProofOfKeags added this to the 0.3.1 milestone May 27, 2022
@@ -8,7 +8,7 @@ if [ "$0" != "./build-prod.sh" ]; then
exit 1
fi

alias 'rust-arm64-builder'='docker run --rm -it -v "$HOME/.cargo/registry":/root/.cargo/registry -v "$(pwd)":/home/rust/src start9/rust-arm-cross:aarch64'
alias 'rust-arm64-builder'='docker run --rm -it -v "$HOME/.cargo/registry":/root/.cargo/registry -v "$(pwd)":/home/rust/src -P start9/rust-arm-cross:aarch64'
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't I remove this?

@@ -154,6 +154,29 @@ pub struct Manifest {
pub dependencies: Dependencies,
}

impl Manifest {
pub fn package_procedures(&self) -> impl Iterator<Item = &PackageProcedure> {
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 I don't like the looks of this but there are no working solutions. I think that a generator syntax would work better.

None
let needs_script = manifest.package_procedures().any(|a| a.is_script());
let has_script = script_path.exists();
match (needs_script, has_script) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Neat. I kinda like this pattern, but if we got more than two, maybe having enums to try and prevent boolean blindness?

match (needs_script, has_script) {
  (AreScripts, HasScripts) => ...,
  (AreScript, MissingScripts) => ...,
  (NoScripts, HasScripts) => ...,
  (NoScripts, MissingScripts) => None,

Copy link
Contributor

@Blu-J Blu-J 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 it

@ProofOfKeags ProofOfKeags merged commit 37344f9 into master May 27, 2022
@MattDHill MattDHill deleted the feature/require-script-consistency branch August 2, 2022 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request P1 - Blocks Release This issue must be addressed before the coming release is shipped start-sdk Issues pertaining to the cli tool for packaging services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants