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

Add feature checkbox #97

Merged
merged 13 commits into from
Jan 6, 2024
Merged

Conversation

DenserMeerkat
Copy link
Contributor

Checkbox for Headers and Parameters #58

lib/codegen/kotlin/okhttp.dart Show resolved Hide resolved
lib/models/request_model.dart Outdated Show resolved Hide resolved
lib/models/request_model.dart Outdated Show resolved Hide resolved
lib/utils/http_utils.dart Outdated Show resolved Hide resolved
test/utils/http_utils_test.dart Outdated Show resolved Hide resolved
@ashitaprasad
Copy link
Member

This is working well @DenserMeerkat
Just a few comments above which can addressed.

@ashitaprasad ashitaprasad added work in progress Dev team is already working on this issue and removed under review labels Dec 22, 2023
@DenserMeerkat
Copy link
Contributor Author

This is working well @DenserMeerkat Just a few comments above which can addressed.

Thanks for the review @ashitaprasad
I've made the changes mentioned in the review and added tests.

@ashitaprasad ashitaprasad added under review and removed work in progress Dev team is already working on this issue labels Dec 23, 2023
@@ -111,9 +111,9 @@ void main() async {
}
Copy link
Member

Choose a reason for hiding this comment

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

enabled params are not being passed/used in http codegen.

rows = [
kNameValueEmptyModel,
];
rows = [kNameValueEmptyModel];
Copy link
Member

Choose a reason for hiding this comment

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

Add , after the first item for singleton lists.

rows = [
kNameValueEmptyModel,
];
rows = [kNameValueEmptyModel];
Copy link
Member

Choose a reason for hiding this comment

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

Add , after the first item for singleton lists.

kNameValueEmptyModel,
];
rows = [kNameValueEmptyModel];
isRowEnabledList = [true];
Copy link
Member

Choose a reason for hiding this comment

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

Add , after the first item for singleton lists.

Copy link
Member

Choose a reason for hiding this comment

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

Do not directly use enabledRequestParams or enabledRequestHeaders in requestModelToHARJsonRequest as it is also used for data export purposes and these should only be used in code gens.
Just like exportMode add a new parameter bool useEnabled and by default it should be false.
The flag should be turned on only when this function is called via code generators.

Copy link
Member

Choose a reason for hiding this comment

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

If it is true, only then enabled fields should be used.

@@ -127,4 +127,39 @@ Easily manipulate and play around with request inputs like headers, query parame
expect(padMultilineString(text1, 10), text1FirstLineNotPaddedExpected);
});
});

Copy link
Member

Choose a reason for hiding this comment

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

These tests are only for getEnabledRows, you will need to add more tests to test the added functionality in more detail.
You need to define 3-4 new models in test/request_models.dart that have list of enabled headers and params (separate & mix of both) and add relevant expected codes in the test/codegen files to ensure the generated codes are correct.

@ashitaprasad
Copy link
Member

Hi @DenserMeerkat,
Thanks for updating the PR. I have reviewed it and added my comments.

@ashitaprasad ashitaprasad added the work in progress Dev team is already working on this issue label Dec 25, 2023
@@ -180,3 +180,91 @@ const requestModelDelete2 = RequestModel(
}""",
requestBodyContentType: ContentType.json,
);

/// Request model with enabled params
const requestModelEnabledParams = RequestModel(
Copy link
Member

Choose a reason for hiding this comment

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

GET request models are named requestModelGet9 ..10 ..
The id should also be get9 ..10..
In the comments above the model we write the purpose.
Same goes for all the below models.
Also, they should be positioned where the last get request model ends.

],
);

/// Request model with enabled rows
Copy link
Member

Choose a reason for hiding this comment

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

Instead of rows write some enabled headers & URL parameters

],
);

/// Request model with disabled rows
Copy link
Member

Choose a reason for hiding this comment

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

Instead of with disabled rows write where all headers & URL parameters are disabled

@@ -100,6 +100,7 @@ void main() {
expect(curlCodeGen.getCode(requestModelPost3, "https"), expectedCode);
Copy link
Member

Choose a reason for hiding this comment

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

For all tests cases .. as they are of GET request .. position them correctly.

@ashitaprasad
Copy link
Member

Hi @DenserMeerkat I have added my comments above. Thanks!

@DenserMeerkat
Copy link
Contributor Author

Thanks @ashitaprasad
For taking your time to review and add comments.
I've made the mentioned changes.

@ashitaprasad ashitaprasad added under review and removed work in progress Dev team is already working on this issue labels Dec 29, 2023
@ashitaprasad
Copy link
Member

Thank you for the PR @DenserMeerkat 🥳
LGTM 🚀

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.

None yet

3 participants