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 .arch function #166

Merged
merged 3 commits into from
Apr 15, 2023
Merged

Add .arch function #166

merged 3 commits into from
Apr 15, 2023

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Apr 13, 2023

Note:

  • I also had a version that wrote out all options

  • We could also implement a stricter type for Arch

  • I think we should also implement a catch-all platform (Unknown(String)) to facilitate adding new platforms easily

/// Return the arch string for the platform
/// The arch is usually the part after the `-` of the platform string.
/// Only for 32 and 64 bit platforms the arch is `x86` and `x86_64` respectively.
pub fn arch(&self) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this without the unwrap? especially if we get “unknown”. Maybe return option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, especially for noarch it should be undefined :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also prefer to write out the match statement. And only use the string split for the unknown case. That way you depend less on the to string implementation which seems a bit cleaner.

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 I do think that's good, but it also makes us have the same data in two places which is what I dislike... but I can write it out and we could add a test or something.

@wolfv
Copy link
Contributor Author

wolfv commented Apr 14, 2023

Just for the record, I've also looked at implementing a custom platform (that would be Custom(String) or so, but didn't yet, because String doesn't implement Copy and so we have to refactor a little more code to use .clone() in some places.

Or is there a copyable string somewhere that we could/should use?

@baszalmstra baszalmstra merged commit 4c0d4d9 into conda:main Apr 15, 2023
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