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

ACN to valory + docs #95

Merged
merged 30 commits into from
Apr 6, 2022
Merged

ACN to valory + docs #95

merged 30 commits into from
Apr 6, 2022

Conversation

Karrenbelt
Copy link

@Karrenbelt Karrenbelt commented Mar 28, 2022

Proposed changes

  1. Changing authorship of ACN connection and protocol.
  2. Updating ACN documentation accordingly.
  3. Updating ACN tests accordingly
  4. Pinning protobuf ==3.19.4 in Pipfile and tox.ini
  5. Updating workflow.yml according to point 4.

Fixes

  • changed authorship on ACN protocol and connection.
  • changed p2p_libp2p* connection versions to 0.1.0
  • updating documentation accordingly and included p2p-connection.md

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 and CI passes too
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that code co- left protocol version at 1.1.0verage does not decrease.
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

docs/p2p-connection.md Outdated Show resolved Hide resolved
docs/p2p-connection.md Show resolved Hide resolved
docs/p2p-connection.md Outdated Show resolved Hide resolved
docs/p2p-connection.md Outdated Show resolved Hide resolved
docs/p2p-connection.md Outdated Show resolved Hide resolved
@Karrenbelt Karrenbelt changed the title [WIP] ACN docs [WIP] ACN to valory + docs Apr 1, 2022
@Karrenbelt Karrenbelt force-pushed the ACN-docs branch 2 times, most recently from ce23608 to bd5a01a Compare April 2, 2022 10:48
Copy link

@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.

Now there's an issue on golang tests. I'll let you fix them.

Pipfile Show resolved Hide resolved
@Karrenbelt Karrenbelt force-pushed the ACN-docs branch 2 times, most recently from 80d1a64 to 6a5b8b9 Compare April 5, 2022 16:15
@Karrenbelt Karrenbelt force-pushed the ACN-docs branch 2 times, most recently from 708db18 to 8805548 Compare April 6, 2022 07:26
@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2022

Codecov Report

Merging #95 (4cf16d5) into main (953ad45) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 4cf16d5 differs from pull request most recent head 019f22b. Consider uploading reports for the commit 019f22b to get more accurate results

@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
+ Coverage   96.89%   96.92%   +0.03%     
==========================================
  Files         307      301       -6     
  Lines       24258    23920     -338     
==========================================
- Hits        23504    23185     -319     
+ Misses        754      735      -19     
Flag Coverage Δ
unittests 96.92% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aea/helpers/serializers.py 100.00% <100.00%> (ø)
packages/valory/protocols/acn/__init__.py
packages/valory/protocols/acn/acn_pb2.py
packages/valory/protocols/acn/custom_types.py
packages/valory/protocols/acn/dialogues.py
packages/valory/protocols/acn/message.py
packages/valory/protocols/acn/serialization.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 953ad45...019f22b. Read the comment docs.

Comment on lines +25 to +28
from google.protobuf.struct_pb2 import ( # pylint: disable=no-name-in-module
ListValue,
Struct,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is # pylint: disable=no-name-in-module required after pinning protobuf version?

Choose a reason for hiding this comment

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

No it's not required anymore

with:
go-version: "^1.14.0"
go-version: "1.17.7"

Choose a reason for hiding this comment

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

We use different versions throughout this file. That's not desirable. I propose we use >=1.14.0 <=1.17.0 throughout as this makes explicit the ranges for which it works.

Choose a reason for hiding this comment

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

Feel free to open an issue for it.

Copy link

@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 changed the title [WIP] ACN to valory + docs ACN to valory + docs Apr 6, 2022
@DavidMinarsch DavidMinarsch merged commit d487fc4 into main Apr 6, 2022
@DavidMinarsch DavidMinarsch deleted the ACN-docs branch April 6, 2022 17:53
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.

4 participants