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

feat: Introduce localkey #233

Merged
merged 5 commits into from
Oct 10, 2022
Merged

feat: Introduce localkey #233

merged 5 commits into from
Oct 10, 2022

Conversation

sitaram-kalluri
Copy link
Member

- What I did

  • Introduce the local keys

- How I did it

  • Introduces new key type - local.

  • Files Modified:

    • at_key.dart:
      - Introduce the static method "local" that returns "LocalKeyBuilder" instance
      - Added a new sub class "LocalKey" which extends AtKey to represent a local key

    • at_key_builder_impl.dart:
      - Added class "LocalKeyBuilder" which extends "AbstractKeyBuilder"

    • key_type.dart:
      - Added "localKey" to the enum

    • at_key_regex_utils.dart:
      - Added regex for local key with and without enforcing the namespace

    • at_key_type_test.dart:
      - Added unit test for local keys

- How to verify it

- Description for the changelog

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

AtKey.toString() and AtKey.fromString() need to work, as they do for other key types. This suggests we need to add a bool isLocal = false to AtKey. Please also add to the test suites in at_key_regex_test.dart and at_key_test.dart

@sitaram-kalluri sitaram-kalluri force-pushed the atkey_local branch 3 times, most recently from 8b6555e to 66cc729 Compare September 27, 2022 08:41
at_commons/lib/src/keystore/at_key.dart Outdated Show resolved Hide resolved
@sitaram-kalluri sitaram-kalluri requested a review from gkc October 7, 2022 03:34
at_commons/lib/src/keystore/at_key.dart Outdated Show resolved Hide resolved
at_commons/lib/src/keystore/at_key.dart Outdated Show resolved Hide resolved
at_commons/lib/src/keystore/at_key.dart Outdated Show resolved Hide resolved
at_commons/test/at_key_regex_test.dart Show resolved Hide resolved
@sitaram-kalluri sitaram-kalluri requested a review from gkc October 10, 2022 10:45
@@ -575,5 +575,27 @@ void main() {
expect(localKey.isLocal, true);
expect(localKey.toString(), 'local:phone@alice');
});

test('validate a local key with namespace', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test description says 'with namespace' but there is no namespace used in this test. And shouldn't the validation then fail because namespace should be mandatory? Same applies to the next test below

Copy link
Member Author

Choose a reason for hiding this comment

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

@gkc, i have copied the test from above and changed the key types. missed correcting the test subject line. Since enforceNamespace defaults to false, the tests did not fail. Added a new test with enforceNamespace set to true which asserts on a failure

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 10, 2022 12:11
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

LGTM

@sitaram-kalluri sitaram-kalluri marked this pull request as ready for review October 10, 2022 12:54
@sitaram-kalluri sitaram-kalluri merged commit 5086dbf into trunk Oct 10, 2022
@sitaram-kalluri sitaram-kalluri deleted the atkey_local branch October 19, 2022 13:12
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.

3 participants