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

pls, review my attempt to fix #706 #707

Closed
wants to merge 3 commits into from
Closed

Conversation

s13o
Copy link
Contributor

@s13o s13o commented Mar 19, 2017

No description provided.

@ctrimble
Copy link
Collaborator

It looks like you need to rebase this change on the current state of master.

@joelittlejohn
Copy link
Owner

Hi @s13o

This change seems to be introducing some specific checks for Integer values but I don't think is the correct way to fix this. There should be an opportunity for recursion here to solve the problem in the general case - getDefaultEnum can use getDefaultValue to ensure that the full range of JExpressions can be supported. I haven't checked this solution but I think it should work 😄

Also I can see quite a few changes that look unrelated. Changes in plugin version, a load of Android related dependencies that I don't understand. Could you move these unrelated changes into their own commits?

@s13o
Copy link
Contributor Author

s13o commented Mar 20, 2017

Guys, I don't really know what are you asking me.
Well, I'm agreed that some new revisions in master has appeared sine I've created my fork but I don't know how to 'rebase the change on the current state of master'.
Any short guideline? Any link?

@s13o
Copy link
Contributor Author

s13o commented Mar 20, 2017

About android test dependencies. There where a lot errors in test like "dependency XXX not found in any repo" but they where resolved immediately when I've added them to "pom.xml" directly. Strange. I thought that it might be related with local maven cache (~/.m2/repo/) that you, guys, have already but mine is empty... dependencies are requested from IT tests but not from maven dependency loading flow... that is some magic, it not it?

@joelittlejohn
Copy link
Owner

joelittlejohn commented Mar 20, 2017

@s13o When you're working to create a pull request, it's important that you create a new branch for each pull request you intend to submit. This makes it a lot easier to modify and add to your changes later since GitHub will understand that the pull request relates to a specific branch and it will keep them in sync. Each time you want to work on a new feature, just pull the latest master and start a new branch. It's good to always keep master in your fork as an exact match to master in the upstream repository (this repo).

Now, lets think about what to do to your repository next. I'd reset my local repo so that master is back in sync with the upstream project, then start working on a branch.

git remote add upstream git@github.com:joelittlejohn/jsonschema2pojo.git
git fetch upstream # pull data about state of upstream branches
git checkout master # ensure you are on master
git reset --hard upstream/master # reset master so that it matches the upstream project
git checkout -b enum-int-default # create a new branch for this PR
git cherry-pick f673fc5 # bring the actual commit you want into this branch
git reset HEAD~1 # start working on your changes again

Now please remove some of the unnecessary changes before you make your commit. I'm happy to bring these in but since they don't relate to the Enum default work in any way they should be separate commits.

When you're happy, commit your changes and use:

git push origin enum-int-default

Then raise a new PR using the enum-int-default branch.

Hope this helps.

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