Skip to content

Conversation

kunal-pmj
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

#8339
Constant Values for the QueryParameter are not taken into account while generating the TypeScript Code-Angular code

@kunal-pmj kunal-pmj changed the title Fix for Issue https://github.com/swagger-api/swagger-codegen/issues/8339 Fix for Issue https://github.com/swagger-api/swagger-codegen/issues/8339 :"Language: Typescript-anglar" Jul 20, 2018
@kunal-pmj kunal-pmj changed the title Fix for Issue https://github.com/swagger-api/swagger-codegen/issues/8339 :"Language: Typescript-anglar" Fix for Issue https://github.com/swagger-api/swagger-codegen/issues/8339 Jul 20, 2018
@kunal-pmj
Copy link
Contributor Author

Any Idea how to assign a reviewer to this Pull Request?

@HugoMario HugoMario self-requested a review July 23, 2018 08:02
@HugoMario
Copy link
Contributor

hello @kunal-pmj , thanks for your PR, i'll take a time to review!

@kunal-pmj
Copy link
Contributor Author

Thanks @HugoMario

@HugoMario
Copy link
Contributor

HugoMario commented Jul 26, 2018

copying same comment here:

Hey again @kunal-pmj, this LGTM, i would just recommend to edit the petstore sample adding this case, only if it's not present. and based on this, update the typescript-angular samples. wdyt?

i could give you a hand if you need help.

@kunal-pmj
Copy link
Contributor Author

Hi @HugoMario

I have added the case to the petstore sample. However, I see that there are many samples across different angular versions. Do I need to regenerate them all with my change?

Thanks
Kunal

Kunal Singh added 2 commits July 30, 2018 11:34
…dified the petstore sample with a new api with the case
@HugoMario
Copy link
Contributor

i think one sample is ok, just to confirm you're changes works fine.

@kunal-pmj
Copy link
Contributor Author

kunal-pmj commented Jul 30, 2018

Hi @HugoMario

All the Typescript sample generators under bin/ and bin/windows directories use the petstore 2.0 specification and NOT the petsore specification with fake path where I had added the api. I tested the code generation for typescript-angular-v4.3 using petsore specification with fake path and I can see the fix working fine, in terms of code generation.
In order keep the samples generators for Typescript in sync with the code fix, Where Should I make the changes?

or

or none of these would be necessary?

Thanks
Kunal

@kunal-pmj
Copy link
Contributor Author

Hi @HugoMario

Can you help by answering questions from my previous update?

Thanks
Kunal

@HugoMario
Copy link
Contributor

hey @kunal-pmj sorry for delay, these have been crazy busy days, i think the best approach is to have another petstore 2.0 specification with your changes and use it at least in one sample.

using petsore specification with fake path is also a good choice, but i think it would required to update samples with changes not related to your fix.

…2. Added case in a seperate petstore specification Updated the test sample by regenerating it with the changes
@kunal-pmj
Copy link
Contributor Author

Hi @HugoMario
Kindly review the changes.
I have added a new petstore specification and updated the typescript angular 4.3 non

Thanks
Kunal

Removed additional space from the file
@kunal-pmj
Copy link
Contributor Author

Tests are green now. removed leading space from the petstore specification

@kunal-pmj
Copy link
Contributor Author

Hi @HugoMario

Can I have your review Comments?

thanks
Kunal

@HugoMario
Copy link
Contributor

sure, i'll take a look on this today and let you know.

@kunal-pmj
Copy link
Contributor Author

Hi @HugoMario

Do you want me to cover some other test?

Thanks
Kunal

@HugoMario
Copy link
Contributor

hi @kunal-pmj LGTM, thanks a lot for your effort and patience, really sorry for this delay

@HugoMario HugoMario merged commit 9544f72 into swagger-api:master Aug 16, 2018
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