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

Fix error in REST Client: Can not find message descriptor by type_url… #287

Closed
wants to merge 4 commits into from

Conversation

Widlar
Copy link

@Widlar Widlar commented Sep 20, 2022

I described this error in the bug report: #284
This little pull request solves the problem.

@5A11
Copy link
Member

5A11 commented Sep 20, 2022

Hey @Widlar, Can you run make lint and address the linting issues please?

@Widlar
Copy link
Author

Widlar commented Sep 20, 2022

Hey @Widlar, Can you run make lint and address the linting issues please?

Hello, thank you for the reminder and sorting imports. I added ignoring 'imported but unused' when calling flake8 in the Makefile. Now make lint passes without errors:

black cosmpy tests examples setup.py --exclude cosmpy/protos
All done! ✨ 🍰 ✨
150 files left unchanged.
isort cosmpy tests examples setup.py
Skipped 1 files
flake8 cosmpy tests examples setup.py --per-file-ignores="__init__.py:F401" 
vulture cosmpy tests examples setup.py --exclude '*_pb2.py,*_pb2_grpc.py' --min-confidence 100

As I described in the error report, this import is not used directly in the code, but it is called implicitly when we try to parse the server response. Because one of the response fields has a type that is not defined, we will always get this error. The linter flake8 cannot correctly recognize the implicit use of the import.

I think this mistake may not be the only one. Because proto messages in general case can contain fields of such types, which were not defined in the calling code. This is the reason why I intervened in the Makefile.
Maybe there is a more elegant solution, but I haven't come up with a better one.

Perhaps we should specify in which file we will ignore imports that are not used. As described here.

And we also need to redo this pull request into a develop brunch.

@5A11
Copy link
Member

5A11 commented Sep 22, 2022

PR still failing CI/CD.

Also please base your PRs against the develop branch.

I think this mistake may not be the only one. Because proto messages in general case can contain fields of such types, which were not defined in the calling code. This is the reason why I intervened in the Makefile. Maybe there is a more elegant solution, but I haven't come up with a better one.

Please add the flake8 exception into setup.cfg instead of directly in the command in Makefile. This makes it easier to track all the exceptions.

@solarw
Copy link
Contributor

solarw commented Oct 24, 2022

fixed with another PR #294
thanks a lot for contributing

@5A11 5A11 closed this Oct 25, 2022
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