Re: [hibernate-dev] [Search] New checkstyle classes working in IDE
by Davide D'Alto
> I think it was a great idea to introduce checkstyle, but I think it
really needs to work in the IDE as wel
Actually for me it is just a nice feature but not a must have.i've never
used checkstyle in the IDE and I'm quite happy to use it in the terminal.
> If we cannot find a solution for this I vote for disabling these custom
checks for now
Does everything work if you remove the rule from the checkstyle.xml? I'm
just asking to see if it is a viable solution.
That said I would prefer to keep the rule even if it doesn't work in the
IDE. Formatters should be already compatiible with the checkstyle file. So
I don't think it will have a huge impact not having it in the IDE.
On Wed, Jul 10, 2013 at 2:08 PM, Hardy Ferentschik <hardy(a)hibernate.org>wrote:
> Hi,
>
> I am wondering whether anyone can use the checkstyle plugin after
> HSEARCH-1326 and the introduction of our own custom Checkstyle Checks
> (Eclipse or Idea)?
> I have been using the Idea Checkstyle plugin and that worked just fine.
> But since the latest changes I get the following exception:
>
> java.lang.IllegalAccessError:
> com/puppycrawl/tools/checkstyle/checks/regexp/CommentSuppressor
> at
> com.puppycrawl.tools.checkstyle.checks.regexp.DoubleSpacesCheck.<init>(DoubleSpacesCheck.java:47)
> at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native
> Method)
> at
> sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:39)
> at
> sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:27)
> at java.lang.reflect.Constructor.newInstance(Constructor.java:513)
> at java.lang.Class.newInstance0(Class.java:357)
> at java.lang.Class.newInstance(Class.java:310)
> at
> com.puppycrawl.tools.checkstyle.PackageObjectFactory.createObject(PackageObjectFactory.java:113)
> at
> com.puppycrawl.tools.checkstyle.PackageObjectFactory.doMakeObject(PackageObjectFactory.java:91)
> at
> com.puppycrawl.tools.checkstyle.PackageObjectFactory.createModule(PackageObjectFactory.java:152)
> at
> com.puppycrawl.tools.checkstyle.TreeWalker.setupChild(TreeWalker.java:161)
> at
> com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:184)
> at
> com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:157)
> at
> com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:184)
> at
> org.infernus.idea.checkstyle.checker.CheckerFactory$CheckerFactoryWorker.run(CheckerFactory.java:312)
>
>
> This seems to be related -
> http://osdir.com/ml/java.audit.checkstyle.devel/2005-10/msg00036.html and
> suggests that there might be class loader issues.
>
> I think it was a great idea to introduce checkstyle, but I think it really
> needs to work in the IDE as well. Obviously we always had problems
> creating a formatting templates for Eclipse respective Idea which generate
> the same result. Since the introduction of Checkstyle that problem was
> alleviated, since I could fix potential problem manually. Now we are back
> to square one.
>
> If we cannot find a solution for this I vote for disabling these custom
> checks for now.
>
> --Hardy
>
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
12 years, 5 months
Retrospective on "Pull Requests": a waste of time?
by Sanne Grinovero
I think we all agree that PRs have many merits: it's almost pointless
to try highlighting them as we're all experienced developers, and I
guess we all have some horror stories from back in the dark ages when
we couldn't use them.
When we moved to git very few of us had some experience with it, but
those who had did propose a solid workflow; we're using this process
now since approximately 2 years, and while I think it's been quite
good, it's time to apply some Retrospective.
I won't annoy you with the long list of benefits, far more interesting
to discuss what I'm considering - in my experience - a strong
drawback:
### Delays ###
Pull requests introduce some delay; sometimes it's short and
reasonable, sometimes it's huge and well needed, but sometimes it gets
very long without providing a significant value to the project.
Actually, since these delays can cost us significant time, there is a
large opportunity cost and not all of us are always aware of what you
might be blocking.
# Context Switch #
Now even if it introduces a strong delay, often it's not a big problem
as we can "context switch" to other subjects or even other projects,
but in practice I'm struggling a lot by this context switch and -
assuming I'm not the only one hit by this - I think we should avoid it
as much as possible: context switch is not just energy draining, it's
also demotivating and has a direct impact on the code quality. That's
huge: code quality is the core of what we're trying to improve with
the pull requests process, but in practice it's achieving the
opposite!
# Real tangible delays #
In some cases, a failed review doesn't just force a context switch but
brings to failed deliveries, or unability to work on the next task. Ok
this is not common, but it happens.
# Conflicts #
This is not a soft-problem nor too hard to handle, but of course the
more we keep parallel branches the more we have conflicts to resolve.
Some of you pointed out it's no big deal but I've often volunteered
for some hard ones... I guess I should change that strategy so you can
better feel the pain ;-)
### Counter-measures ###
I don't think it's worth cancelling the PRs process as there are great
benefits, but it's worth exploring how to minimize the undesired
side-effects.
I also don't think we can nail down precise "rules" .. I will just ask
to use common sense and try hard to merge pulls quickly.
To add some specific directions: I noticed frequently some good points
are made on a PR which are not strictly a blocker to the pull itself.
Of course if the tests fail or if it introduces other blockers which
prevent other developers to continue on their work, it should not be
merged.. but in most other cases we should try to ask for "follow up"
improvements rather than "fix now".
If we can create follow-up improvements, we can easily prioritize
them. Decoupling the priority of further improvements from the main
goal of the pull request is important to make sure that we're spending
time on the highest value instead of entertaining ourselves with
design puzzlers.. I know the feeling, we all are scientists and enjoy
some nitty gritty polishing but often there's no real gain from it,
not even long term readability as I'd rather hope our code to evolve
faster.. we're not polishing a statue of immutable stone.
Also, a follow-up improvement can easily get reassigned to someone
else; for example often the contributor pointing out a possible
improvement has a better idea of the change he would like to see
applied, so he could "just code it" and propose it as a change.
Finally, we could try to *reject* PRs more often. If you think there
is a blocking problem with the pull, today we would usually ask for
improvements and wait for the puller to fix, we could as well make it
very clear when we think that it's a blocker problem by rejecting it..
I suspect that could have some good effects:
- no push-force on the same pull hiding previous dicussions
- potentially a different reviewer on the next attempt (we had cases
on pulls gettting stuck because the single reviewer was away!)
- no long lists of dormant pull requests
I think this would also encourage people to be quick in merging if
there is *any* open pull request.
The problem here is to not take it as an offence, but I think if we
were all doing it more regularly, - if it becomes a normal process
rather than an exception - then there would be no problem with it.
Possibly we would need to explain that nicely to newcomer
contributors!
# Avoid pull requests for previews
This is becoming more frequent but I came to dislike pulls which stay
open for long (days). I'd prefer to keep the queue empty and try some
alternatives, like sending pull requests to specific developer's
rather than to the main project. This way you could "invite" specific
devs, and they could close the pulls to signal they consider the
review task complete.. or explore something like that.
# Avoid showing off scanner capabilities :-)
I've been very guilty myself on this one; I remember occasionally to
have attacked a PR with a sense of "let's see how many nitty gritty
details I can find", then effectively pointing out many typos and
small inaccuracies but possibly failing to evaluate the grand plan. I
hope I've learned to "let go" a bit.. for these cases I think it's of
course great to comment on github but use good judgment to see if it's
worth delaying the merge. Often I find myself applying minor fixed
myself, some other times, I expect follow up polishing pulls, or we
open new issues.
# When to delay?
Consider that a sequence of PRs will show up in history as a sequence
of commits, and it's going to be impossible to figure out from the
master history if the changes are caused by a single PR or by multiple
PRs.
Consider also that when asking a PR improvement, we often stack
additional commits...
I really prefer if we all could "fixup" the wrong commits, but in case
there is no need for it, you could as well merge and ask for further
improvements.. could be a good rule of thumb?
Let's fight our love for perfection and bring home some *useful* stuff
ASAP. (Hint: renaming internal classes is not in this category)
-- Sanne
12 years, 5 months
[Search] New checkstyle classes working in IDE
by Hardy Ferentschik
Hi,
I am wondering whether anyone can use the checkstyle plugin after HSEARCH-1326 and the introduction of our own custom Checkstyle Checks (Eclipse or Idea)?
I have been using the Idea Checkstyle plugin and that worked just fine. But since the latest changes I get the following exception:
java.lang.IllegalAccessError: com/puppycrawl/tools/checkstyle/checks/regexp/CommentSuppressor
at com.puppycrawl.tools.checkstyle.checks.regexp.DoubleSpacesCheck.<init>(DoubleSpacesCheck.java:47)
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:39)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:27)
at java.lang.reflect.Constructor.newInstance(Constructor.java:513)
at java.lang.Class.newInstance0(Class.java:357)
at java.lang.Class.newInstance(Class.java:310)
at com.puppycrawl.tools.checkstyle.PackageObjectFactory.createObject(PackageObjectFactory.java:113)
at com.puppycrawl.tools.checkstyle.PackageObjectFactory.doMakeObject(PackageObjectFactory.java:91)
at com.puppycrawl.tools.checkstyle.PackageObjectFactory.createModule(PackageObjectFactory.java:152)
at com.puppycrawl.tools.checkstyle.TreeWalker.setupChild(TreeWalker.java:161)
at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:184)
at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:157)
at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:184)
at org.infernus.idea.checkstyle.checker.CheckerFactory$CheckerFactoryWorker.run(CheckerFactory.java:312)
This seems to be related - http://osdir.com/ml/java.audit.checkstyle.devel/2005-10/msg00036.html and suggests that there might be class loader issues.
I think it was a great idea to introduce checkstyle, but I think it really needs to work in the IDE as well. Obviously we always had problems
creating a formatting templates for Eclipse respective Idea which generate the same result. Since the introduction of Checkstyle that problem was
alleviated, since I could fix potential problem manually. Now we are back to square one.
If we cannot find a solution for this I vote for disabling these custom checks for now.
--Hardy
12 years, 5 months
Re: [hibernate-dev] Retrospective on "Pull Requests": a waste of time?
by Hardy Ferentschik
I guess we have been talking up requests from within the team. At least I have. IMO they are generally a different beast with other things to consider.
--hardy
On 9 Jul 2013, at 22:48, Steve Ebersole <steven.ebersole(a)gmail.com> wrote:
> I am curious if y'all are talking about all pull requests, whether that be from community members as well as those from within the team?
>
> On Tue 09 Jul 2013 02:39:12 PM CDT, Hardy Ferentschik wrote:
>> I basically like what I hear. Some wise words :-)
>>
>> On 9 Jan 2013, at 9:00 PM, Emmanuel Bernard <emmanuel(a)hibernate.org> wrote:
>>
>>> There has been a tendency to let PR sit a bit longer than it should as
>>> we all try to get our stuff done before diving into other's PRs.
>>> I have been particularly guilty and Hibernate OGM is a particularly bad
>>> example. I did not see too much lagging PRs on other projects
>>
>> Right. I feel Search for example is working quite well.
>>
>>> Should we have a team member on watch whose priority for a week is
>>> addressing pull requests?
>>
>> Not sure. The idea has some merits, but I am not sure that it is necessary.
>>
>>> I also think opening issues to things that are not fully on the topic is
>>> a good strategy to keep the cycle on a given PR short.
>>
>> What is the definition of not fully on topic. I would not suggest a change in
>> class X for a pull request where only class Y and Z are affected. However,
>> if class X is touched and I see a potential improvement I think it can be considered
>> being part of the topic. Boy Scout rule number one:" Always leave the campground
>> cleaner than you found it." I truly believe in this one, but of course sometimes a
>> potential improvement would have too big of a ripple effect to be pursued.
>>
>>> That does not
>>> mean one won't work directly on these after the PR is done
>>
>> Here we have to disagree. Unless you do it here and now the chances are slim
>> you are following up.
>>
>>> About the preview, I have to disagree with Sanne and Hardy, I do like
>>> them and find them to be the least worse tool to show a preview and get
>>> feedback. I'm sorry but I have done it many times on JIRA and via emails
>>> and the feedback is not the same by far.
>>
>> My experience with asking for feedback on my feature branches is actually
>> quite good. Kudos to Gunnar and Sanne on their helps. I guess if I felt I would be
>> left hanging on this type of feedback, I would create these "preview" pull requests
>> as well.
>>
>>> Like many rules, they are meant to be broken and good judgement is highly
>>> preferable
>>
>> That's the part I like best.
>> +100
>>
>> --Hardy
>> _______________________________________________
>> hibernate-dev mailing list
>> hibernate-dev(a)lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
12 years, 5 months
Dynamic-map and one-to-one relation mapped with intersection table
by Łukasz Antoniak
Hello all,
While trying to fix one of Envers issues, I have encountered difficulties
inserting one-to-one relation mapped with join table. All audit entities
generated by Envers follow EntityMode.MAP pattern. Simple hibernate-core
test case - https://gist.github.com/lukasz-antoniak/5929345 - fails because
INSERT statement populating WIFE_HUSBAND table precedes creation of HUSBAND
record. The workaround would be to skip populating referenced record id in
the entity that is saved first (comment out line "wifeRecord.put(
"husband_id", 2l );"). Would you classify such Hibernate behavior as a bug?
I can see advantages of both approaches - simplicity (current situation)
vs. compatibility with classic POJO behavior.
Regards,
Lukasz
12 years, 5 months
[PARSER] project rename
by Gunnar Morling
Hi all,
As discussed with some of you before, the experimental query parser project
has been renamed to accommodate the fact that it is a parser for HQL
queries and not only JPQL.
That's the changed URL:
* old: https://github.com/hibernate/hibernate-jpql-parser
* new: https://github.com/hibernate/hibernate-hql-parser
Assuming you've named your remote for the master repo "upstream", you can
change the remote's URL like this:
git remote set-url upstream git(a)github.com:
hibernate/hibernate-hql-parser.git
I recommend to rename your fork on GitHub as well (via the "settings" icon
on the lower right on the main view of your fork). Assuming you've named
the remote for your fork "origin", you can then change this remote's URL
like so:
git remote set-url origin git(a)github.com
:<YOUR_USER>/hibernate-hql-parser.git
The change also reflects in the group/artifact ids, e.g. it's
org.hibernate.hql:hibernate-hql-parser:<VERSION> now.
Last but not least, the parser project is built using Gradle now. Thanks to
the "Gradle wrapper" which is part of the project, you don't have to
install anything for that, you can run any build command via ./gradlew.
Upon first usage, this will download the actual Gradle tool. Some useful
commands:
* ./gradlew clean build (complete build, roughly the same as "mvn clean
package")
* ./gradlew publishToMavenLocal (install artifacts to local Maven
repository)
* ./gradlew publish (deployment to Nexus, think "mvn deploy")
Let me know in case should any issues arise due to these changes.
--Gunnar
12 years, 5 months
