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

Use better typing, mostly relating to DatasetVersionDifference (#775 again) #2423

Closed
wants to merge 2 commits into from

Conversation

bencomp
Copy link
Contributor

@bencomp bencomp commented Jul 31, 2015

In addition to defining and instantiating variables as typed lists and command, and using typed queries, I refactored some Object[]s to SummaryData and BlockData types and updated the references in the JSF. See the commit messages.

bencomp added 2 commits July 31, 2015 11:02
Instead of using `Object[]` with various types and needing knowledge of
the type and meaning of the members of the array, use SummaryData and
BlockData.

Also, don't use raw types and unchecked conversions.
Manually added diamond operators in files.
@scolapasta scolapasta added this to the 4.2 milestone Aug 3, 2015
@scolapasta scolapasta modified the milestones: 4.3, 4.2 Sep 18, 2015
@pdurbin
Copy link
Member

pdurbin commented Sep 30, 2015

@bencomp and I mentioned at #2478 (comment) this pull request is against the 4.2 branch, which has already been tagged. It should be made against 4.3 instead.

Also, from chatting with @sekmiller it seems like this pull request is too long to easily reason about. I also wonder if you've tested these changes. :)

Please keep the pull requests coming! It's great stuff!

@bencomp
Copy link
Contributor Author

bencomp commented Sep 30, 2015

When I made the pull request, 4.2 was the active branch. It's too bad I have to wait about two months to hear "it's risky".

These changes mostly add static typing (because Object arrays have been replaced by proper and meaningful classes and raw Lists etc. were analysed for their usage and made typed) and have been tested through ongoing static analysis in NetBeans and successful compilation by javac. I did not test the refactored JSF, but I felt confident about these changes because of @scolapasta's training.

Please note that in many places in the code methods are too long and documentation is too poor to reason about what things actually do, really. Unfortunately it has the effect that refactoring creates big changes. If you like, I can try to explain a few things via Skype or chat.

@scolapasta
Copy link
Contributor

Hi Ben @bencomp ,

I'll send you an e-mail shortly, and coordinate with you our plan to review this and get the changes in.

@pdurbin
Copy link
Member

pdurbin commented Oct 20, 2015

@bencomp speaking of active branches, we now have two active branches:

  • 4.2.1
  • 4.3

This is because in 583ace0 I created the 4.2.1 branch based on work done in 4.3.

#2589 is against 4.3 which is perfect. We want to release 4.2.1 with as few changes as possible. I wouldn't want to put the changes in that pull request through QA for 4.2.1.

Even though we have two active branches, as of this writing the "default" branch is still 4.3. Why? My preference is that the "default" branch should be the branch where we want pull requests. That's why I haven't changed the "default" branch to 4.2.1.

@bencomp I'm curious what you think about all this. I'm thinking about writing a section about the "default" branch at http://guides.dataverse.org/en/latest/developers/branching-strategy.html to clarify why we make the default branch what it is.

(Another option is to create a "develop" branch that's always the "default" branch but I have been reluctant to create a "develop" branch because of the tendency for developers to never want to get off it as I explained in detail at #2478 (comment) .)

@bencomp
Copy link
Contributor Author

bencomp commented Oct 20, 2015

@pdurbin I agree with you on keeping 4.3 the default branch.

I'll close this PR and open a new one but not right away. My time is limited ;)

@pdurbin pdurbin removed this from the 4.3 milestone Oct 29, 2015
@pdurbin
Copy link
Member

pdurbin commented Oct 29, 2015

@bencomp as discussed at #2478 this pull request has been made against an old branch (4.2) that should not receive any more commits. There's no way to switch the branch so I have to close the pull request. Sorry.

@pdurbin pdurbin closed this Oct 29, 2015
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.

4 participants