Skip to content

support for params having multiple values #681

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

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

badnikhil
Copy link
Contributor

@badnikhil badnikhil commented Mar 20, 2025

PR Description

The request parameters were previously handled as a Map<String, String>. I have updated this to Map<String, List<string> to support multiple values

Additionally, I have modified the relevant implementations to accommodate this change.

Related Issues

Checklist

  • I have reviewed the contributing guide.
  • My branch is up to date and synced with the project's main branch before creating this PR.
  • I am using the latest stable Flutter version (flutter upgrade verified).
  • [] I have run all tests (flutter test), and they pass successfully.

Added/Updated Tests?

We encourage contributors to add relevant test cases.

  • Yes
  • No, because I plan to add tests after finalising the changes.

Development & Testing Environment

  • Windows
  • Android
  • Linux

@badnikhil
Copy link
Contributor Author

badnikhil commented Mar 20, 2025

@ashitaprasad I am familiar with the codebase; however, I need to explore the test files further before making the necessary updates. I plan to work on that after updating codegen for all languages. In the meantime, please review the implementation and share your feedback.

@badnikhil badnikhil changed the title Resolve issue 166 support for params having multiple values Mar 20, 2025
@badnikhil badnikhil mentioned this pull request Mar 23, 2025
6 tasks
@badnikhil
Copy link
Contributor Author

badnikhil commented Apr 4, 2025

@animator I request you to take a look(Feedback on the major changes is enough) . I've updated the code generation for all supported languages to handle parameters with multiple values. Before I move on to writing/updating tests, I just want to ensure that all changes are aligned with expectations .

@@ -34,14 +34,28 @@ class HttpRequestModel with _$HttpRequestModel {
_$HttpRequestModelFromJson(json);

Map<String, String> get headersMap => rowsToMap(headers) ?? {};
Map<String, String> get paramsMap => rowsToMap(params) ?? {};
Map<String, dynamic> get paramsMap => rowsToRequestMap(params) ?? {};
Copy link
Member

Choose a reason for hiding this comment

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

params map should have a strict datatype - Map<String, List>

List<NameValueModel>? get enabledHeaders =>
getEnabledRows(headers, isHeaderEnabledList);
List<NameValueModel>? get enabledParams =>
getEnabledRows(params, isParamEnabledList);

Map<String, String> get enabledHeadersMap => rowsToMap(enabledHeaders) ?? {};
Map<String, String> get enabledParamsMap => rowsToMap(enabledParams) ?? {};
Map<String, dynamic> get enabledParamsMap {
Copy link
Member

Choose a reason for hiding this comment

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

same for enabled params

@@ -12,7 +12,7 @@ class RequestDataTable extends StatelessWidget {
this.valueName,
});

final Map<String, String> rows;
final Map<String, dynamic> rows;
Copy link
Member

Choose a reason for hiding this comment

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

This can no longer be a map

@animator
Copy link
Member

animator commented Apr 6, 2025

@badnikhil do not conversations as resolved. You have to comment if you have fixed it, then the reviewer verifies the fix and marks it is resolved.

@badnikhil
Copy link
Contributor Author

@badnikhil do not conversations as resolved. You have to comment if you have fixed it, then the reviewer verifies the fix and marks it is resolved.

Sorry about that, I’ll keep it in mind from now on.

@badnikhil
Copy link
Contributor Author

badnikhil commented Apr 10, 2025

@animator i have resolved the datatypes.. Also updated a widget test.
rows are now List instead of map
you can Ignore the codegen related warnings for now.. it will be updated after finalising the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not pass multiple query parameters with same key name
2 participants