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.

Sign up for GitHub

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

Jump to bottom

Keep track of the original ProjectDescription #1023

Closed
wants to merge 1 commit into from
Closed

Keep track of the original ProjectDescription #1023

wants to merge 1 commit into from

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Oct 22, 2019 •
edited
Loading

No description provided.

@onobc onobc mentioned this pull request Oct 22, 2019
Closed
snicoll
snicoll reviewed Oct 22, 2019
View reviewed changes
Copy link
Contributor

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the prototype. It's good to see some of the principles in code. I think the main problem right now is the lack of support of custom ProjectDescription. I've added a few comments.

...or/src/main/java/io/spring/initializr/generator/project/ProjectDescriptionModifications.java Outdated Show resolved Hide resolved
...or/src/main/java/io/spring/initializr/generator/project/ProjectDescriptionModifications.java Outdated Show resolved Hide resolved
...or/src/main/java/io/spring/initializr/generator/project/ProjectDescriptionModifications.java Outdated Show resolved Hide resolved
@snicoll
Copy link
Contributor

snicoll commented Oct 23, 2019

Thanks for the update but you don't really need to introduce this right now I think. We already have an integration test with a custom project description. I'll do my best to review when time permits.

@onobc
Copy link
Contributor Author

onobc commented Oct 23, 2019 •
edited
Loading

When you review you will see my comment around the "custom" one and that it was just there for the prototype so I can see what it would look like when doing the custom project description.
I did see the whole custom package and how the request is converted into the custom desc etc...

Here are my main questions when you get around to reviewing:

  1. Do we want a custom diff object that has typed isXyzChanged methods
  2. What will it look like when we construct the above (if we do).
  3. Is the copy constructor being called in the right place?

@onobc
Copy link
Contributor Author

onobc commented Oct 24, 2019

@snicoll I did not get back around to this today (work keeping me busy). I would not review it yet. I did some thinking today and I have the answers I need. I will ping you when I go another round on it.

@onobc
Copy link
Contributor Author

onobc commented Oct 24, 2019

Hi @snicoll - ok lets start fresh. I have rebased and this setup works for both regular and custom diffs. Please take a peek when you can. I will drop a few comments in the MR as well. Thanks.

@onobc
Copy link
Contributor Author

onobc commented Oct 25, 2019

@snicoll Was wanting to knock this out over the weekend. Hopefully you can take a peek and give a high-level review before I moved forward anymore. Thanks. If you can't - I understand - you are busy on many things.

@onobc
Copy link
Contributor Author

onobc commented Oct 26, 2019 •
edited
Loading

@snicoll I think this is a good 1st cut at the diff model. It supports default project description as well as the custom one. I have added tests as well. I think this is ready for full review.

@onobc onobc force-pushed the gh-1005 branch 2 times, most recently from c933acd to cd8dfd0 Compare October 27, 2019 04:03
@onobc onobc changed the title WIP: gh-1005: Prototype approach gh-1005: Add project description diff Oct 27, 2019
@onobc
Copy link
Contributor Author

onobc commented Nov 4, 2019 •
edited
Loading

Such a lonely little PR out here w/ nobody to talk to. 😜

@mbhave
Copy link
Contributor

mbhave commented Nov 4, 2019

@bono007 Thank you for taking the time to update the PR. The team is currently quite tied up with Spring Boot work and reviewing PRs for initializr might take a little longer than usual. We have not forgotten about it and we will get to it in due course :) Thank you for your patience.

@onobc
Copy link
Contributor Author

onobc commented Nov 4, 2019

No worries @mbhave - thx for heads up. Also, I have been looking for some issues in BOOT to tackle so if you run into something that would be good to dole out, please let me know. Thx

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 12, 2019
@snicoll
Copy link
Contributor

snicoll commented Nov 26, 2019

I've had a good look at this one while traveling and it is heading in the right direction. I hope to be able to find some time to finalize the polish commit I've started. Thanks!

@snicoll snicoll added type: enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 26, 2019
@snicoll snicoll added this to the 0.9.0 milestone Nov 26, 2019
This was referenced Dec 27, 2019
Closed
Closed
@snicoll snicoll changed the title gh-1005: Add project description diff Keep track of the original ProjectDescription Dec 27, 2019
@snicoll snicoll self-assigned this Dec 27, 2019
snicoll pushed a commit that referenced this pull request Dec 27, 2019
@cbo-indeed @snicoll
Keep track of original ProjectDescription
161fccb 
See gh-1023
snicoll added a commit that referenced this pull request Dec 27, 2019
@snicoll
Polish "Keep track of original ProjectDescription"
5325e67 
See gh-1023
@snicoll snicoll closed this in 175ed1b Dec 27, 2019
@snicoll
Copy link
Contributor

snicoll commented Dec 27, 2019

Thanks @bono007, this is now merged with a polish commit. I've removed the final modifiers that you added as it's not consistent with the rest of the code base. I also moved the code outside of the diff package as your proposal introduced a package tangle.

@onobc
Copy link
Contributor Author

onobc commented Dec 27, 2019

Your welcome @snicoll . Yeh the final is a work-related holdover :)

All looks good. Now we can use this to do things like #1006

snicoll added a commit that referenced this pull request Dec 27, 2019
@snicoll
Make sure ProjectDescriptionDiff is created eagerly
6c261fd 
This commit makes sure that the diff instance is created immediately,
taking a snapshot of the ProjectDescription before the customizers are
applied on it.

Previously, the instance was created as part of the supplier and
therefore created only once a component was requiring it.

See gh-1023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

Assignees

@snicoll snicoll

Labels
type: enhancement
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@onobc @snicoll @mbhave @spring-projects-issues @cbo-indeed