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

Randomly generated IDs #931

Merged
merged 20 commits into from
Sep 26, 2023
Merged

Randomly generated IDs #931

merged 20 commits into from
Sep 26, 2023

Conversation

philpax
Copy link
Contributor

@philpax philpax commented Sep 21, 2023

Closes #679, closes #921.

The code for the CLI was rearranged because my initial plan was to move all of the package commands into a package subcommand, and then set up top-level aliases, but I ran into a few hurdles and decided to leave it as-is for now.

I won't automerge this until I have signoff from the reviewers because I suspect we'll want to wipe the database before we do.

}

#[derive(Args, Clone, Debug)]
pub struct HostCli {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this one is left here because of the hurdles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's just the common args for the commands that host (run, serve). Can't do much about that, I think.


#[derive(Parser, Clone, Debug)]
/// Builds and runs the package locally
pub struct Run {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the idea is for run to be under package? ambient package run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that I'd move all the commands there, and then have aliases back to them, as-is the case with campfire - so that you could always have a consistent interface, but also a convenient one. However, didn't happen, so it's still just ambient run.

ALPHABET[idx - NUMERALS.len()]
};
// TODO: see if there's a more intelligent way to ensure the first character
// is always alphabetic
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost like BTC mining 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comparison was not lost on me and it makes me sad 😂

@philpax philpax enabled auto-merge September 22, 2023 20:03
@philpax philpax disabled auto-merge September 25, 2023 11:25
@philpax
Copy link
Contributor Author

philpax commented Sep 26, 2023

Waiting for #961 to go in, then merging

@Pombal
Copy link
Contributor

Pombal commented Sep 26, 2023

Worth retesting #909 if the CLI code changed.

@philpax
Copy link
Contributor Author

philpax commented Sep 26, 2023

I think that issue is with the version-manager-CLI, not the Ambient CLI (yes this is confusing, see AmbientRun/AmbientCli#3)

@philpax philpax merged commit 2ae01fb into main Sep 26, 2023
@philpax philpax deleted the randomly-generated-ids branch September 26, 2023 18:48
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.

Remove ambient view Switch to randomly-generated IDs for packages
4 participants