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

[Scala] Fix missing json4s import #7271

Merged
merged 2 commits into from
Jan 7, 2018
Merged

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Dec 29, 2017

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. (/cc @clasnake @jimschubert)

Description of the PR

Closes #7270

@gmarz gmarz changed the title Fix/7270 [Scala] Fix missing json4s import Dec 29, 2017
Copy link
Contributor

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

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

Obviously a needed change!

We usually avoid importing all types like this, for fear of conflict with API models (this is why the default formats reference is fully qualified), but I doubt there will be conflicts with types defined in json4s. If that ends up being an issue, we can change to use fully qualified type names later.

@gmarz
Copy link
Contributor Author

gmarz commented Jan 3, 2018

We usually avoid importing all types like this, for fear of conflict with API model

@jimschubert yea, good point. I wasn't sure about this, but ended up convincing myself that json4s._ was better to avoid this from potentially happening again in the future if we introduce other json4s types in the template...but I see your argument for being restrictive as well. I'm happy to change it if you feel strongly about it.

@jimschubert
Copy link
Contributor

Nah, I don't think it needs a change unless we get feedback that it's causing issues. My comment was more to track that I don't foresee conflicts being likely with this import for json4s

@wing328 wing328 added this to the v2.3.1 milestone Jan 7, 2018
@wing328 wing328 merged commit 14e7158 into swagger-api:master Jan 7, 2018
@wing328
Copy link
Contributor

wing328 commented Jan 7, 2018

@gmarz thanks for the fix, which has been merged into master (2.3.1)

@jimschubert thanks for reviewing the change.

jimschubert added a commit to jimschubert/swagger-codegen that referenced this pull request Jan 10, 2018
* master: (26 commits)
  [Scala] Fix async helper methods when body is optional (swagger-api#7274)
  [Rust] Recommend style based on 'rustfmt' defaults (swagger-api#7335)
  [Java:vertx] Initialize router in init method and re-use router member to create S… (swagger-api#7234)
  [Scala] Fix missing json4s import (swagger-api#7271)
  deploy snapshot version 2.3.1
  [Ada] Add Ada support for server code generator swagger-api#6680 (swagger-api#7256)
  add shijinkui to scala technical committee
  Generate swagger yaml for go client (swagger-api#7281)
  use openjdk7 in travis to ensure it works with jdk7
  docs(readme): update link to contributing guid (swagger-api#7332)
  Fix a regression bug that was introduce in a recent commit. Removed the tabs that were causing error in Play Framework (swagger-api#7241)
  Fix issue swagger-api#7262 with the parameter name in the path. The problem was that camelCase naming was forced only in this part of the code when everywhere else it is configurable. (swagger-api#7313)
  Java8 fix (swagger-api#7260)
  update to 2.3.1-SNAPSHOT
  fix typo, update 2017 to 2018
  [Doc] add huawei cloud to companies list swagger-api#7308 (swagger-api#7309)
  Adding Peatio opensource as reference project (swagger-api#7267)
  Update README.md (swagger-api#7298)
  Update README.md (swagger-api#7299)
  [all] sys props in CodegenConstants
  ...
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.

3 participants