Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support qemu in GitHub Actions #482
Support qemu in GitHub Actions #482
Changes from 22 commits
76fe472
90a14b9
ec99c01
0d29bf8
26b7b72
0c9af03
f73b608
7c86223
5aa85e8
0ca294e
91800e4
27f1a4e
8a1a0a4
b34bdee
eb03a11
3696f15
68dc286
3983fb2
460eaa7
f6ce1c9
6b00931
0893731
b5f5437
5ee663f
f434072
d65da2e
5b5078e
aa87128
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going from top to bottom, so I haven't seen yet what this is, but
a.name for a in Architecture
looks weird.Architecture
tells me it's a type, and you're looping over a type?(I'll figure out what's going on soon enough, but this is my first reaction ;-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected, an enum.
That's horrible syntax IMO, looping over a type, but apparently how Python envisions you doing things, so why not, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
a.value
rather thana.name
, though, to match the logic fromArchitecture.parse_config()
(even though, yes, names and values are the same) ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a "type", it's an enum. How else would you loop over all the possible enums? C++ doesn't have this capability (yet, at least, until reflection is available?). Keep in mind adding any methods to Enum would remove possible Enum values! And yes, I think it should be
name.value
.It acts like a container of enum instances, so looping over it is pretty reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, an enum is a type, no? Both in the broad sense (we're using it to tell mypy what's passed) as well as in the Python sense where
enum.EnumMeta
is a subclass oftype
.But yeah, I can see it's a special one. Just haven't seen this that often and the capital/syntax highlighting is quite confusing; nothing this PR should do anything about, though!
Reiterating that
a.name
->a.value
is still a change to be made, just to make sure the serious part of my comments stands out ;-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code,
CIProvider
really should not be a StrEnum - there's no need for that. I'm rather thinkingArchitecture
shouldn't be either. It is not needed to subclassstr
to use strings as values, but only adds implicit conversion. I don't see any reason that we can't use explicit conversion here.Though the implicit form does allow this beauty:
How's that for a "type"? :)
BTW, an enum should be thought of as both a type and a container of all the instances of that type. The fact that is is a type is actually a bit of a workaround in leu of true dedicated language support, for example
Architecture.count
is thestr.count
method; you can't add a "count" member to a StrEnum (at least without breaking its stringiness).@joerick what do you think? Should I remove
str,
from both of these and make them regular enums with string-based values, or do you want me to add:and then use that, making these full StrEnums? Type safety/explicit or shorter/implicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, still horrible, but better than before? ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking the code might actually be clearer if we used plain old enums, rather than the StrEnum thing. Now we have type-checking, we end up having to be strict with them anyway. And
enum.value
is clearer and more explicit.