-
Notifications
You must be signed in to change notification settings - Fork 80
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 support for a_out_picture action #351
Added support for a_out_picture action #351
Conversation
2b57385
to
ae6d514
Compare
dcs/action.py
Outdated
class PictureToUnit(PictureAction): | ||
predicate = "a_out_picture_u" | ||
|
||
def __init__(self, unit: int, file_res_key: ResourceKey, seconds: int, |
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.
Same as the previous two, but for unit
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 is now unit_id -- if you can help me solve circular import, I don't mind changing it to proper types.
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.
@DanAlbert - unit_id to Unit can't be done without refactoring the code which parses triggers. At the moment trigger/action parsing code is unaware of the context of coalition/country it's parsing for, therefore unit_id, since it's not unique, cannot be resolved simply. I'd rather keep this as is and refactor the code later that focuses on adding context to trigger/action parsers, then refactor this piece to use it. Thoughts?
|
dcs/action.py
Outdated
Center = "1" | ||
Right = "2" | ||
|
||
def __eq__(self, other) -> bool: |
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 surprised to see __eq__
defined for these. Are these actually needed?
If they are, other: Any
(on all of them)
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.
Are these needed then? Could you explain why? It looks superfluous.
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.
@DanAlbert, I'd like to use strong types in tests. I can remove this, but is there a way we'd be able to use strong types in tests like: self.assertEqual(m_action.horz_alignment, PictureAction.HorzAlignment.Left) -- looking it from a point of view that I load a mission in Mission Editor, see the names, like Left, and I open a test and I say "aha! understood". If I see "1" in test I have to open .miz and then say "aha! understood. Let me know what you want me to do.
PS: I don't like using enum.value approach -- and I know it's valid, but it's ugly IMO.
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.
Ah, I get it now. You want https://docs.python.org/3/library/enum.html#enum.StrEnum.
Other than switching to |
* supports sending picture to all, coalition, country, group and unit. DCS UI issue allows picture to be sent to unit without picture resource, so a dedicated test covers that.
* Used country_id, group_id, and unit_id due to circular dependencies in action.py
* moved importing units before parsing triggers and actions so groups can be resolved to objects * refactored PictureToCountry to use County object rather than country_id * refactored PictureToGroup to use Group object rather than group_id * added implementation of find_group_by_id to mission, country, coalition to support lookup needed when creating a PictureToGroup action * Updated test .miz (added more units to confirm groupIds are unique, assigned units to either USA or Third Reich)
* Updated type hints on PictureTo* Actions
* raising exception rather than asserting
* replaced assert with type hint
* explicit not None checks
* type hint updated for find_group_by_id
@DanAlbert - as I read StrEnum is available in python11. Tests are running using 3.8, 3.9, 3.10, 3.11. I'm using python 3.10 and StrEnum is not available. Should I leave it until 3.11 is required or what's do you suggest? |
5dccfb1
to
08544bd
Compare
Ah, good catch. I didn't realize that was so new. Yeah, keep with what you've got. |
This change allows WWIIBigFormation to use Carpet Bombing task. Removed duplicate tests for big formation which crept in with PR #351
supports sending picture to all, coalition, country, group and unit. DCS UI issue allows picture to be sent to unit without picture resource, so a dedicated test covers that.
This change allows WWIIBigFormation to use Carpet Bombing task. Removed duplicate tests for big formation which crept in with PR pydcs#351
This change allows WWIIBigFormation to use Carpet Bombing task. Removed duplicate tests for big formation which crept in with PR pydcs#351
NOTE: assert isinstance is used to make mypy happy. If there are better ways, please let me know.