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/API-86: generate eclipse files based on the config flag #286

Merged
merged 1 commit into from
May 8, 2019

Conversation

santoshaherkar
Copy link
Member

@santoshaherkar santoshaherkar commented Apr 17, 2019

No description provided.

@santoshaherkar santoshaherkar changed the title Fix/api 86 Fix/API-86: generate eclipse files based on the config flag Apr 17, 2019
@stevehu stevehu requested review from GavinChenYan and ddobrin April 17, 2019 20:20
@santoshaherkar
Copy link
Member Author

@stevehu I had selected base branch as master but in actual PR it shows release.. is that because default branch is release?

I have added tag in pom file to enable snapshots downloading and added sample config.json as well, just to get an idea about how that file should be.

@stevehu
Copy link
Contributor

stevehu commented Apr 17, 2019

@santoshaherkar It is great to have this flag to control if the eclipse files are generated or not. There are some existing users expecting these files to be generated with their existing config.json file. I am wondering if we should make this flag default to true in the generator. I have include @ddobrin @ChenYan71 as they have some developers using eclipse. What do you think?

@santoshaherkar santoshaherkar changed the base branch from release to master April 17, 2019 20:50
Copy link
Contributor

@ddobrin ddobrin left a comment

Choose a reason for hiding this comment

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

I agree with the idea of the default, to be left to true, as there are a number of projects in flight

@santoshaherkar
Copy link
Member Author

@NicholasAzar do you agree with keeping this flag to true by default? Jira item mentions that eclipse file should not be created by default, so just wanted to confirm.

@DSchrupert
Copy link
Member

Currently i dont agree with that, as both eclipse and intellij can import projects just based on the pom.xml.. and the eclipse files actually just cause problems with intellij. Having them generated as default seems incorrect to me.
That being said if there's a push to keep this unchanged for whatever reason, then unfortunately we'll have to add another flag.

@stevehu
Copy link
Contributor

stevehu commented Apr 18, 2019

@NicholasAzar These files were introduced in the early days of light-codegen. At that time, there were only several configuration options available and nobody ever thought to add a flag to enable/disable it. I am using IntelliJ IDEA and never encounter any problem with these two files. Did anybody report issues with these files? What was the issue? Thanks.

@stevehu
Copy link
Contributor

stevehu commented May 7, 2019

@ddobrin I think you added these two files in the first place. Are you OK to not generate these files by default? Can eclipse generate these files by importing pom.xml?

@ddobrin
Copy link
Contributor

ddobrin commented May 7, 2019

@stevehu: we can just set the default to false and that's it.
"eclipseIDE": false,

Or, we can set the default value to false in the code and, if somebody sets the flag, it can be generated.

Eclipse, unfortunately has issues with importing pom files.

If you guys don't wish to have this at all, please update the issue here for confirmation and I will address it separately for my client.

@stevehu
Copy link
Contributor

stevehu commented May 8, 2019

@ddobrin Thanks Dan for the confirmation. The implementation is exactly what you have described.

@stevehu stevehu merged commit 444a4ec into master May 8, 2019
@stevehu
Copy link
Contributor

stevehu commented May 8, 2019

@stevehu stevehu deleted the fix/API-86 branch May 8, 2019 14:33
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.

4 participants