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

AEA-245 AEA-244 AEA-225 Adding protocol generator to the CLI tool #605

Merged
merged 14 commits into from
Jan 11, 2020

Conversation

5A11
Copy link
Member

@5A11 5A11 commented Jan 6, 2020

Proposed changes

Adding the protocol generator form the agents-research repository to the this repository.

Adding protocol generation as an option in the CLI tool.

Fixes #521, #556, #557

Types of changes

What types of changes does your code introduce to agents-aea?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that code coverage does not decrease.
  • I have checked that the documentation about the aea cli tool works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Work in progress...

@5A11 5A11 changed the base branch from master to develop January 6, 2020 14:02
Copy link
Contributor

@DavidMinarsch DavidMinarsch left a comment

Choose a reason for hiding this comment

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

Great work as a first step. Please consider my comments and add some tests for the new command.

aea/cli/core.py Show resolved Hide resolved
aea/cli/generate.py Outdated Show resolved Hide resolved
aea/cli/generate.py Outdated Show resolved Hide resolved
aea/cli/generate.py Outdated Show resolved Hide resolved
aea/cli/generate.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
…hortening error checking code in protocol template class; replacing 0.1.0 with a constant DEFAULT_VERSION;
Copy link
Contributor

@DavidMinarsch DavidMinarsch left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, spotted a few more suggestions.

aea/cli/generate.py Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
aea/protocols/sample_protocol_specification.yaml Outdated Show resolved Hide resolved
…otocolConfig object; Addresses second bacth of comments in PR; changing authors->author; license_config->license; moved sample_proto_spec file to new location;
aea/cli/generate.py Outdated Show resolved Hide resolved
aea/protocols/generator.py Outdated Show resolved Hide resolved
…elCase for class names; list of contents -> dictionary of contents and styled;
… tests pass; CURRENTLY: 1 test does not pass (commented); Not sure about the validity of TestGenerateProtocolFailsWhenExceptionOccurs, although it passes;
Copy link
Contributor

@DavidMinarsch DavidMinarsch left a comment

Choose a reason for hiding this comment

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

Great work on tests! I have some comments re the protocols we generate. We can improve the formatting and make sure they are mypy/flake8 compliant. We need to fix the contents argument. A list won't work as we have a dict elsewhere in speech_acts so there is no ordering.

tests/data/generator/two_party_negotiation/message.py Outdated Show resolved Hide resolved
tests/data/generator/two_party_negotiation/message.py Outdated Show resolved Hide resolved
tests/data/generator/two_party_negotiation/message.py Outdated Show resolved Hide resolved
tests/data/generator/two_party_negotiation/message.py Outdated Show resolved Hide resolved
tests/data/generator/two_party_negotiation/message.py Outdated Show resolved Hide resolved
if message_id == 1:
assert target == 0, "target should be 0"
else:
assert 1 < target < message_id, "target should be between 1 and message_id"
Copy link
Contributor

Choose a reason for hiding this comment

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

assert 0 < target < message_id, "target should be strictly between 0 and message_id"

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the first message's target can be 0 (i.e. targets nothing since first move).
The other moves must target an existing one (i.e. 1 < target < message_id).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but your code excludes target=1 which is a valid target for a message_id > 1

tests/test_cli/test_generate/test_protocols.py Outdated Show resolved Hide resolved
tests/test_cli/test_generate/test_protocols.py Outdated Show resolved Hide resolved
tests/test_cli/test_generate/test_protocols.py Outdated Show resolved Hide resolved
Copy link
Contributor

@DavidMinarsch DavidMinarsch left a comment

Choose a reason for hiding this comment

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

LGTM

@DavidMinarsch DavidMinarsch merged commit a473ca3 into develop Jan 11, 2020
@DavidMinarsch DavidMinarsch deleted the add/generator branch January 11, 2020 12:47
@DavidMinarsch DavidMinarsch changed the title Adding protocol generator to the CLI tool AEA-245 AEA-244 AEA-225 Adding protocol generator to the CLI tool Jan 11, 2020
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