Skip to content

Conversation

InfoSec812
Copy link
Contributor

Resolves quarkusio/quarkusio.github.io#326

The verbiage in the Repository section of the Panache guide, while explicitly stating "we're not judging", comes across as judgemental/condescending to some people. This is a minor adjustment to the tone of the language used so it can appeal to a broader audience.

So if Repositories are your thing, you can keep doing them. Even with repositories, you can keep your entities as
subclasses of `PanacheMongoEntity` in order to get the ID and public fields working, but you can even skip that and
go back to specifying your ID and using getters and setters if that's your thing. We're not judging.
subclasses of `PanacheEntity` in order to get the ID and public fields working, but you can even skip that and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subclasses of `PanacheEntity` in order to get the ID and public fields working, but you can even skip that and
subclasses of `PanacheMongoEntity ` in order to get the ID and public fields working, but you can even skip that and

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks for the review!

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

One error and maybe the sentence about repository can be simplified

So if Repositories are your thing, you can keep doing them. Even with repositories, you can keep your entities as
subclasses of `PanacheMongoEntity` in order to get the ID and public fields working, but you can even skip that and
go back to specifying your ID and using getters and setters if that's your thing. We're not judging.
subclasses of `PanacheEntity` in order to get the ID and public fields working, but you can even skip that and
Copy link
Contributor

Choose a reason for hiding this comment

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

subclasses of PanacheMongoEntity

Comment on lines 336 to 339
Repositories can be amazing or evil, depending on the complexity of your needs. When they work,
they can save you from writing lots of boilerplate and error-prone JPA/SQL/Criteria queries,
but when they fail you need a way to drop back to more traditional methods... Don't worry,
Panache and Quarkus have you covered either way.
Copy link
Contributor

Choose a reason for hiding this comment

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

The first sentence feels a bit long and still has some judgment. Maybe going straigth to the point should be bettre.

Repository is a very popular design and can be very accurate for some use case, Panache and Quarkus have you covered ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it! Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still thinking that the following sentence is not needed:

but when you need to support a complex use case you can revert
to more traditional methods...

And the Don't worry seems to miss some introduction

If you need them, don't worry ...

@geoand
Copy link
Contributor

geoand commented Oct 31, 2019

@InfoSec812 Thanks for the PR! One thing we usually ask contributors to do is squash so a single commit (or into more logical commits if it makes sense) before merging in order to get rid of commits addressing review comments and such.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Can you have a look at my comments? Thanks!

Look, we get it: you have a love/hate relationship with DAOs/Repositories but you can't live without them. We don't judge, we
know life is tough and we've got you covered.
Repository is a very popular pattern and can be very accurate for some use case, depending on
the complexity of your needs. When your use case fits, they can save you from writing lots of boilerplate
Copy link
Member

Choose a reason for hiding this comment

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

What is they?

Copy link
Member

Choose a reason for hiding this comment

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

Reading from the comments, I think the first sentence was modified and the second sentence was not and is definitely not accurate anymore.

So if Repositories are your thing, you can keep doing them. Even with repositories, you can keep your entities as
subclasses of `PanacheEntity` in order to get the ID and public fields working, but you can even skip that and
go back to specifying your ID and using getters and setters if that's your thing. We're not judging.
go back to specifying your ID and using getters and setters if that's your thing. Use what works for you.
Copy link
Member

Choose a reason for hiding this comment

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

+1


Look, we get it: you have a love/hate relationship with DAOs/Repositories but you can't live without them. We don't judge, we know life is tough and we've got you covered.
Repository is a very popular pattern and can be very accurate for some use case, depending on
the complexity of your needs. When your use case fits, they can save you from writing lots of boilerplate
Copy link
Member

Choose a reason for hiding this comment

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

Same question :).

@machi1990 machi1990 added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Nov 1, 2019
@InfoSec812
Copy link
Contributor Author

@InfoSec812 Thanks for the PR! One thing we usually ask contributors to do is squash so a single commit (or into more logical commits if it makes sense) before merging in order to get rid of commits addressing review comments and such.

@geoand I've never really developed a habit of using rebase/squash because I prefer the added details in my commit history. Is there a command which you recommend to make it simple?

@geoand
Copy link
Contributor

geoand commented Nov 1, 2019

@InfoSec812 I usually use IntelliJ's build in UI which makes things super easy. Otherwise git's interactive rebase (as explained here for example) is what I occasionally use

@gsmet gsmet force-pushed the Issue_326-_-Adopt_a_different_tone_for_Panache_repository_guide branch from 3d11dea to 6855891 Compare November 3, 2019 23:03
@gsmet
Copy link
Member

gsmet commented Nov 3, 2019

I made a few additional adjustments and rebased. I think it's good to go now but will wait for CI.

@gsmet gsmet added this to the 0.28.0 milestone Nov 3, 2019
@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Nov 3, 2019
@stuartwdouglas stuartwdouglas merged commit 5c4fa5a into quarkusio:master Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/documentation triage/waiting-for-ci Ready to merge when CI successfully finishes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panache repository guide comes across as condescending....

6 participants