Skip to content

Conversation

jlevitt
Copy link
Contributor

@jlevitt jlevitt commented Jun 8, 2014

Bug fix for https://nhibernate.jira.com/browse/NH-3455

This is ready for code review. Please let me know if there's anything I might be overlooking, as this is my first contribution and I'm extremely new to this codebase.

Questions:

  • Does this mean that projection.GetColumnAliases(propertyName, 0); is buggy, or was it just being misused?

@jlevitt jlevitt closed this Jun 8, 2014
@hazzik
Copy link
Member

hazzik commented Jun 8, 2014

Why colsed?

@jlevitt
Copy link
Contributor Author

jlevitt commented Jun 9, 2014

@hazzik

I didn't mean to submit this yet. I've only got the failing test case so far, I haven't fixed the issue. This is my first time contributing, so I'm not really sure what the process is :) Also, I wasn't sure if I should merge to master in my fork before submitting a PR, or if the PR should be directly from my topic branch.

Also, is there any way for me to update the Jira issue to let people know I'm working on it?

@hazzik
Copy link
Member

hazzik commented Jun 9, 2014

I've replied to your email to dev mailing list. Basically this is it:

  1. To let people know you can write a comment on JIRA issue (need to register first)
  2. If you want to make PR only with tests - that is perfectly fine, but please mark those failing tests with [KnownBug] attribute, so CI would expect them to fail. If the test come with fixes, then you dont need to mark tests with [KnownBug].
  3. You create a branch for your fixes from master, and then submit pull request from that branch.

@hazzik hazzik reopened this Jun 9, 2014
@jlevitt
Copy link
Contributor Author

jlevitt commented Jun 9, 2014

Ok, thanks for the update. I've added the known bug attribute, so I guess you can merge this test case. I can create a new branch for the fix. Thanks for the help!

@hazzik
Copy link
Member

hazzik commented Jun 9, 2014

You can work on the same branch. Merging is a looong & time consuming process )

@jlevitt
Copy link
Contributor Author

jlevitt commented Jun 22, 2014

I think this is ready for code review/merging. Do I need to merge into master in my fork as well? What is the code review/merge process like?

@jlevitt jlevitt changed the title Add test case for NH3455. Bug fix for NH3455. Jun 22, 2014
@jlevitt
Copy link
Contributor Author

jlevitt commented Jul 11, 2014

@hazzik any update on pulling this?

@hazzik
Copy link
Member

hazzik commented Jul 15, 2014

Sorry, I'm on vacation with limited internet ability

@jlevitt
Copy link
Contributor Author

jlevitt commented Jul 15, 2014

@hazzik ok, thanks for the update. Enjoy your vacation!

@oskarb
Copy link
Member

oskarb commented Jul 25, 2014

@jlevitt You should probably never merge anything into master or any of the other main branches (e.g 3.4.x) that mirror the official repository. It is likely to just confuse matters if you then later create a new topic branch that you want to submit. Just update master etc. from the official repository as needed, and do all your own commits on other branches.

@oskarb oskarb merged commit 3abfaa6 into nhibernate:master Jul 27, 2014
@jlevitt jlevitt deleted the nh3455 branch December 24, 2014 19:45
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.

3 participants