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

Reorg project to match Maven Java standards #38

Merged
merged 21 commits into from
Jun 20, 2017
Merged

Conversation

davidrpugh
Copy link
Member

@adrian-carro This PR will reorganize the housing model code so that it matches Maven standard project template..

@davidrpugh
Copy link
Member Author

@adrian-carro We need to create a pom.xml file that contains the dependencies for the housing model. Once we have a pom file we should be able to delete the /lib directory containing these jar files. No reason that we should be version controlling these files.

@davidrpugh
Copy link
Member Author

@adrian-carro After the reorg it seem to be a lot off issues with class trying to access private/protected data of other classes. Doesn't look like there are any getters (and in some case setters) for the relevant properties.

@adrian-carro adrian-carro self-requested a review May 27, 2017 11:11
@adrian-carro
Copy link
Member

@davidrpugh Thanks for doing this. Please, let me know whenever this PR is ready (up and running) for me to review and merge it. In any case, I'll need some time before merging it, since there are a number of people using this code and I want to make sure I make everybody aware of this reorganisation before proceeding.
Regarding the setters and getters, you are completely right, but bear in mind that 1) this is a pretty large piece of code with very few getters/setters when it was inherited, 2) that the organisation of the different classes might change in the near future as we will be trying to reduce the number of references between multiple objects, and 3) that a deep cleaning and refactoring of the code has not been the priority so far (though we obviously understand it is badly needed!).

@davidrpugh
Copy link
Member Author

@adrian-carro Thanks for the background info. Will let you know when the PR is ready to merge. I will need to write getters (and possibly setters) in order for the reorg to work. This will probably be a painful merge for everyone using the code but it is definitely necessary.

@davidrpugh
Copy link
Member Author

@adrian-carro I have added the necessary getters and changed the file paths to the data (which now lives in the resources directory). I compiled and ran the model and it looked OK but you should see if everything looks good to you.

Still need to finish the pom.xml file and delete the '/lib' folder.

@davidrpugh
Copy link
Member Author

@adrian-carro This PR is ready to merge. Everything now works on my machine not that I have removed the /lib directory containing the external .jar files. All necessary dependencies that were available via Maven have been moved to the pom.xml file and will be automatically downloaded when the project is built by Eclipse or Intellij (or whatever IDE you are using for your work).

Unfortunately, recent versions of Mason are not available on Maven. In the past we had been VC the Mason jar file. I went ahead and deleted the jar file for consistency and also to possibly help with licensing issues. If you think it is too onerous on users to have them install Mason separately in order to use our code, then we can always add the /lib/mason.19.jar file back into this branch prior to the merge.

Feedback is much appreciated...

@adrian-carro
Copy link
Member

Great! In the following days (as soon as I find some time to do it) I'll review it, check that everything works fine in a couple of common IDEs, warn whoever is working with the code, and merge the changes.

@davidrpugh
Copy link
Member Author

@adrian-carro I expect that there will be IDE issues due to preexisting IDE project files. I deleted all of the Intellij and Ecplise IDE specific project files and then created a fresh Maven project using the housing model source code. Let me know if you get into trouble...

@davidrpugh
Copy link
Member Author

@adrian-carro Ping! Just looking for an update to the status of this PR. I have some free time this weekend and would like to get this merged so that I can move on to cleaning up other parts of the code base.

@adrian-carro
Copy link
Member

@davidrpugh Sorry about the delay, I've been busy with other stuff, but this is my priority for today!

@adrian-carro
Copy link
Member

Hey @davidrpugh, I've just been checking this pull request and everything looks pretty much fine to me with the exception of MASON. Indeed, in order to be allowed to compile and run the code, I had to add the mason jar file somewhere in the project, then use maven to install it locally, and finally add it to the pom file as a (local) dependency... is there any simpler way to solve this? Looking forward, I'm pretty confident we will end up dropping mason, given that we are not really using it for anything but to make the code less readable. But still, we won't drop it just now, so we need a simple way for users to run the code using mason. Any ideas/thoughts on how to achieve this? In case there are some steps that users have to follow to install mason, maybe the best idea is to include a clear description of those steps in the Readme.md file? Once this is solved, I will directly merge the changes.

@davidrpugh
Copy link
Member Author

@adrian-carro I have just added the Mason jar back into the repo in the /lib folder. I am trying to figure out how to add it to the poml.xml as a local dependency. Any ideas?

@davidrpugh
Copy link
Member Author

davidrpugh commented Jun 19, 2017

@adrian-carro Can you test drive this and see if it works? I am having some weird Windows issues on my machine that I think is an issue with my laptop and not with this fix...but do let me know if you run into issues...for reference here was the instructions I followed...everything seems to work on my end now. All I needed to do was press the play button to get the GUI charts to start working properly...

@adrian-carro
Copy link
Member

Oh, perfect! It works out the box now. If you are fine with it, I'll just merge the changes.

@davidrpugh
Copy link
Member Author

davidrpugh commented Jun 19, 2017 via email

Copy link
Member

@adrian-carro adrian-carro left a comment

Choose a reason for hiding this comment

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

Great work! In the future we might be able to drop the mason patch, but for now it does the job. Also, the getters and setters still lacking can be progressively added along with future changes.

@adrian-carro adrian-carro removed the request for review from DavoudTaghawiNejad June 20, 2017 09:16
@adrian-carro adrian-carro merged commit a8a9359 into master Jun 20, 2017
@adrian-carro adrian-carro deleted the project-reorg branch June 20, 2017 09:19
@davidrpugh davidrpugh restored the project-reorg branch May 28, 2018 09:41
@adrian-carro adrian-carro deleted the project-reorg branch July 10, 2018 23:12
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.

None yet

2 participants