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

PLAT-10565 Adding utils classes #150

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

symphony-mariacristina
Copy link
Contributor

@symphony-mariacristina symphony-mariacristina commented Mar 16, 2021

Ticket

PLAT-10565

Description

Adding some util classes on message processing to:

  • Extract entities from a given incoming message (mentions, hashtags, cashtags, emojis)
  • Extract message content from presentationML
  • Pre-process an outgoing message by "cleaning" the text message, escaping all special characters that will violate messageML format
  • Convert a stream id into a url safe id and viceversa

Checklist

  • Referenced a ticket in the PR title and in the corresponding section
  • Filled properly the description and dependencies, if any
  • Unit tests updated or added
  • Docstrings added or updated
  • Updated the documentation in docs folder

Adding some util classes on message processing to:
- Extract entities from a given incoming message (mentions, hashtags, cashtags, emojis)
- Extract message content from presentationML
- Pre-process an outgoing message by "cleaning" the text message, escaping all special characters that will violate messageML format
- Convert a stream id into a url safe id and viceversa

Also Pylint was complaining about exceptions definition so I fixed the warning as suggested here: https://stackoverflow.com/a/2901085
@symphony-mariacristina symphony-mariacristina requested a review from a team March 16, 2021 12:10
from symphony.bdk.gen.agent_model.v4_message import V4Message


def get_text_content_from_message(message: V4Message):
Copy link
Member

Choose a reason for hiding this comment

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

I need it in my current PR !! :)

Copy link
Contributor

@symphony-elias symphony-elias left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -7,6 +7,7 @@ class AuthInitializationError(Exception):
"""

def __init__(self, message: str):
Exception.__init__(self, message)
Copy link
Contributor

@symphony-youness symphony-youness Mar 16, 2021

Choose a reason for hiding this comment

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

Curious question: Why do you call the parent constructor for Exception ?
.
.
On internet forums, the usual answer would be super().__init__ instead of Exception.__init__ as it would avoid us using the base class name explicitly.

I however don't see the point of calling the parent constructor if we set the instance attributes.
If you check the source code for Exception:

class Exception(BaseException):
    """ Common base class for all non-exit exceptions. """
    def __init__(self, *args, **kwargs): # real signature unknown
        pass

It only instanciates them for us. So my take would be droping the super().__init__ completly: https://docs.python.org/3.8/tutorial/errors.html -> 8.5. User-defined Exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated these exceptions following the discussion on this page: https://stackoverflow.com/questions/2901000/the-correct-way-to-define-an-exception-in-python-without-pylint-complaining/2901085#290108 .
Pylint was complaining and raising warnings about it, so I followed this comment https://stackoverflow.com/a/2901085 and seems like Pylint is happy now.

But if there's a better way on how to handle this I'm open to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

These discussions seem a bit old (dates of 2010), since python 3+, you would see things more like this: super().__init__: https://stackoverflow.com/questions/1319615/proper-way-to-declare-custom-exceptions-in-modern-python

I explained how I see things and how it was documented in the official documentation. Calling the parent constructor is not doing anything anyways which I think we should remove it.
https://docs.python.org/3.8/tutorial/errors.html -> 8.5. User-defined Exceptions

try:
if item["type"] == entity_type.value:
tags_list.append(item["id"][0]["value"])
except KeyError:
Copy link
Contributor

Choose a reason for hiding this comment

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

if this error case can happen a lot maybe we would be better checking for it explictly (I don't know if it is similar to Java but exceptions have a performance cost and sometimes you want to avoid them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data object coming as payload from the message should be well formed in general. I put this block just to be safe that we are handling this scenario.

@symphony-mariacristina symphony-mariacristina merged commit c6e4d46 into finos:2.0 Mar 17, 2021
symphony-elias added a commit that referenced this pull request Apr 6, 2021
* PLAT-10433: Sphinx documentation (#130)

* PLAT-10483 - HealthService implementation (#132)

- HealthService implementation and tests 
- Renamed test classes from `test_name_of_class.py` to `name_of_class_test.py`

* PLAT-10532: ApplicationService implementation (#133)

* ApplicationService implementation

* Update documentation

* PLAT-10530: Presence service implementation (#134)

- Presence service implementation and tests
- Put the presence service test payloads in one JSON file
- Added type-hint to the ServiceFactory.get_health_service function

* PLAT-10530 Fixed documentation issues in links regarding the presence service (#135)

This fix was added to correctly show links to the endpoint documentation when sphinx documentation is generated.

* PLAT-10531 Signal service implementation (#138)

* PLAT-10529 Ability to set private key and certificate content programmatically (#137)

Private key and Certificate were able to only be loaded from the config file when initializing bdk. In this PR, we update bot config to be able to set private key and certificate after bdk being initialized.

* PLAT-10489: Implemented automatic pagination (#139)

* PLAT-10643: Load certs from system store

In #122 we enable loading a custom cert store for the HTTP client. This
broked using pods with valid certs because the system certs are no
longer loaded. This change loads them even all the time (and we might
add custom ones on top of that).

Also sync the rest.mustache template used for code generation.

* PLAT-10643: Unit test system certs loading

* Added forgotten default value in list_all_stream_members (#144)

* PLAT-10533: Added User-Agent header in requests (#141)

* PLAT-10600 enforce pylint checks in PR builder (#146)

Goal of this PR is to enable pylint checks in the PR builder.
For now the limit to fail is set to 9.50 (max being 10.00). We can start like this and if needed we can adapt it later on, if we think that is too strict.

All changes about fixture are related to this issue: https://stackoverflow.com/questions/46089480/pytest-fixtures-redefining-name-from-outer-scope-pylint

* PLAT-10534: Added X-Trace-Id header to each HTTP call (#147)

* PLAT-10534: Added X-Trace-Id header to each HTTP call

* Replaced single quotes with double quotes

* Fixed python version in GH workflow

* Updated caching logic in workflows (#148)

* Changed caching logic of poetry deps to be specific to the actual python version.

* Updated build, push and pylint to cache pip dependencies as well

* PLAT-10651: Updated generated code following role type update to string (#149)

* Updated generated code using openApiGen 5.0.1 and updated role format to str

* Updated unit tests and examples

* PLAT-10565 Adding utils classes (#150)

Adding some util classes on message processing to:

Extract entities from a given incoming message (mentions, hashtags, cashtags, emojis)
Extract message content from presentationML
Pre-process an outgoing message by "cleaning" the text message, escaping all special characters that will violate messageML format
Convert a stream id into a url safe id and viceversa

* PLAT-10535: Extension App RSA authentication (#153)

* PLAT-10535: First implementation of RSA ext app authentication

* Added and improved docstrings

* Fixed some pylint errors

* Updated poetry deps

* #143 Activity API (#151)

* #143 Activity API (draft version)

* #143 Added unit tests

* #143 Added markdown doc

* #143 Documented AbstractActivity class

* PLAT-10709: Switch from python-jose to PyJWT (#159)

Switch from using python-jose to using PyJWT

* PLAT-10588: Implemented concurrent DF loop (#158)

* PLAT-10588: Implemented concurrency of event handling in DF loop

* Refactored datafeed loops

* Updated poetry deps

* PLAT-10563: FormReply activity (#161)

- Base classes for a FormReply Activity.
- Modification of the command activity to parse attributes in the constructor instead 
- Fixed related tests

* PLAT-10538: Minor improvements on extension app authenticator (#163)

* PLAT-10538: added allowed audience = app-id when validating jwt

* Improved error handling in ExtensionAppAuthenticatorRsa.validate_jwt

* Updated dependencies

* Bumped version to 2.0b1

* PR builder should build 2.0-rc branch

Co-authored-by: symphony-youness <76746033+symphony-youness@users.noreply.github.com>
Co-authored-by: symphony-hong <65538951+symphony-hong@users.noreply.github.com>
Co-authored-by: Mariacristina De Dominicis <65179248+symphony-mariacristina@users.noreply.github.com>
Co-authored-by: Soufiane Aourinmouche <52406574+symphony-soufiane@users.noreply.github.com>
Co-authored-by: Youri Bonnaffe <youri.bonnaffe@symphony.com>
Co-authored-by: Youri Bonnaffé <63661676+symphony-youri@users.noreply.github.com>
Co-authored-by: Thibault Pensec <39826516+symphony-thibault@users.noreply.github.com>
symphony-youness added a commit to symphony-youness/symphony-api-client-python that referenced this pull request Jun 4, 2021
* PLAT-10433: Sphinx documentation (finos#130)

* PLAT-10483 - HealthService implementation (finos#132)

- HealthService implementation and tests 
- Renamed test classes from `test_name_of_class.py` to `name_of_class_test.py`

* PLAT-10532: ApplicationService implementation (finos#133)

* ApplicationService implementation

* Update documentation

* PLAT-10530: Presence service implementation (finos#134)

- Presence service implementation and tests
- Put the presence service test payloads in one JSON file
- Added type-hint to the ServiceFactory.get_health_service function

* PLAT-10530 Fixed documentation issues in links regarding the presence service (finos#135)

This fix was added to correctly show links to the endpoint documentation when sphinx documentation is generated.

* PLAT-10531 Signal service implementation (finos#138)

* PLAT-10529 Ability to set private key and certificate content programmatically (finos#137)

Private key and Certificate were able to only be loaded from the config file when initializing bdk. In this PR, we update bot config to be able to set private key and certificate after bdk being initialized.

* PLAT-10489: Implemented automatic pagination (finos#139)

* PLAT-10643: Load certs from system store

In finos#122 we enable loading a custom cert store for the HTTP client. This
broked using pods with valid certs because the system certs are no
longer loaded. This change loads them even all the time (and we might
add custom ones on top of that).

Also sync the rest.mustache template used for code generation.

* PLAT-10643: Unit test system certs loading

* Added forgotten default value in list_all_stream_members (finos#144)

* PLAT-10533: Added User-Agent header in requests (finos#141)

* PLAT-10600 enforce pylint checks in PR builder (finos#146)

Goal of this PR is to enable pylint checks in the PR builder.
For now the limit to fail is set to 9.50 (max being 10.00). We can start like this and if needed we can adapt it later on, if we think that is too strict.

All changes about fixture are related to this issue: https://stackoverflow.com/questions/46089480/pytest-fixtures-redefining-name-from-outer-scope-pylint

* PLAT-10534: Added X-Trace-Id header to each HTTP call (finos#147)

* PLAT-10534: Added X-Trace-Id header to each HTTP call

* Replaced single quotes with double quotes

* Fixed python version in GH workflow

* Updated caching logic in workflows (finos#148)

* Changed caching logic of poetry deps to be specific to the actual python version.

* Updated build, push and pylint to cache pip dependencies as well

* PLAT-10651: Updated generated code following role type update to string (finos#149)

* Updated generated code using openApiGen 5.0.1 and updated role format to str

* Updated unit tests and examples

* PLAT-10565 Adding utils classes (finos#150)

Adding some util classes on message processing to:

Extract entities from a given incoming message (mentions, hashtags, cashtags, emojis)
Extract message content from presentationML
Pre-process an outgoing message by "cleaning" the text message, escaping all special characters that will violate messageML format
Convert a stream id into a url safe id and viceversa

* PLAT-10535: Extension App RSA authentication (finos#153)

* PLAT-10535: First implementation of RSA ext app authentication

* Added and improved docstrings

* Fixed some pylint errors

* Updated poetry deps

* finos#143 Activity API (finos#151)

* finos#143 Activity API (draft version)

* finos#143 Added unit tests

* finos#143 Added markdown doc

* finos#143 Documented AbstractActivity class

* PLAT-10709: Switch from python-jose to PyJWT (finos#159)

Switch from using python-jose to using PyJWT

* PLAT-10588: Implemented concurrent DF loop (finos#158)

* PLAT-10588: Implemented concurrency of event handling in DF loop

* Refactored datafeed loops

* Updated poetry deps

* PLAT-10563: FormReply activity (finos#161)

- Base classes for a FormReply Activity.
- Modification of the command activity to parse attributes in the constructor instead 
- Fixed related tests

* PLAT-10538: Minor improvements on extension app authenticator (finos#163)

* PLAT-10538: added allowed audience = app-id when validating jwt

* Improved error handling in ExtensionAppAuthenticatorRsa.validate_jwt

* Updated dependencies

* Bumped version to 2.0b1

* PR builder should build 2.0-rc branch

Co-authored-by: symphony-youness <76746033+symphony-youness@users.noreply.github.com>
Co-authored-by: symphony-hong <65538951+symphony-hong@users.noreply.github.com>
Co-authored-by: Mariacristina De Dominicis <65179248+symphony-mariacristina@users.noreply.github.com>
Co-authored-by: Soufiane Aourinmouche <52406574+symphony-soufiane@users.noreply.github.com>
Co-authored-by: Youri Bonnaffe <youri.bonnaffe@symphony.com>
Co-authored-by: Youri Bonnaffé <63661676+symphony-youri@users.noreply.github.com>
Co-authored-by: Thibault Pensec <39826516+symphony-thibault@users.noreply.github.com>
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.

5 participants