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: Remove metadata fields in verb builders #471

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

sitaram-kalluri
Copy link
Member

@sitaram-kalluri sitaram-kalluri commented Dec 7, 2023

- What I did

  • Remove AtKey fields and metadata fields from the verb builders. Use AtKey instance in "AbstractVerbBuilder" to set the key.
  • Since "key" in AtKey cannot be null, removed "?" which indicates that key can be null, and marked it as late.
  • Removed static const String atOperation = 'operation from "at_constants.dart" file because it is redundant.
  • Remove "?" for "isBinary" and "isEncrypted" fields default them to false.
  • In AtClient pacakge, AtKey.Metadata is verififed for null check and if null assigned to Metadata() . Initialized Metadata field to remove the null check in at_client methods.
  • Since metadata is removed from verb builders, removed packages/at_commons/lib/src/verb/metadata_using_verb_builder.dart file which is not needed.
  • Uptake the at_commons changes into at_lookup.
  • Modify the tests in at_commons.

Raised draft PR's in at_client_sdk and at_server to validate the changes:

at_server: atsign-foundation/at_server#1688

at_client: atsign-foundation/at_client_sdk#1179

@murali-shris
Copy link
Member

in verb builders replace string concatenation with string builder

@@ -94,10 +94,11 @@ class AtLookupImpl implements AtLookUp {
Future<bool> delete(String key,
{String? sharedWith, bool isPublic = false}) async {
var builder = DeleteVerbBuilder()
Copy link
Member

Choose a reason for hiding this comment

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

at_lookup changes you can move to separate PR.
at_lookup dependency on at_commons 4.x is a minor version release for at_lookup?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Sure Murali, WIll move the at_lookup changes to a new PR.
  • At_lookup can pusblished as a minor version because we are not introducing any breaking changes.

@sitaram-kalluri
Copy link
Member Author

in verb builders replace string concatenation with string builder

This change is completed and pushed the changes.

murali-shris
murali-shris previously approved these changes Dec 11, 2023
@sitaram-kalluri sitaram-kalluri merged commit cb87f0c into trunk Dec 11, 2023
11 checks passed
@sitaram-kalluri sitaram-kalluri deleted the srk_commons_changes branch December 11, 2023 10:26
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