Timestamps are in GMT/BST.
[6:48] -orwell.freenode.net- *** Looking up your hostname...
[6:48] -orwell.freenode.net- *** Checking Ident
[6:48] -orwell.freenode.net- *** Found your hostname
[6:48] -orwell.freenode.net- *** No Ident response
[6:48] * DuraLogBot (~PircBot@webster.duraspace.org) has joined #duraspace
[6:48] * Topic is 'Welcome to DuraSpace IRC. This channel is used for formal meetings and is logged - http://irclogs.duraspace.org/'
[6:48] * Set by tdonohue on Thu Sep 15 17:49:38 UTC 2016
[7:06] * kompewter (~kompewter@50.17.201.82) Quit (Read error: Connection reset by peer)
[7:06] * DSpaceSlackBot (~DSpaceSla@ec2-50-17-201-82.compute-1.amazonaws.com) Quit (Read error: Connection reset by peer)
[7:20] * DSpaceSlackBot (~DSpaceSla@ec2-50-17-201-82.compute-1.amazonaws.com) has joined #duraspace
[12:32] * tdonohue (~tdonohue@dspace/tdonohue) has joined #duraspace
[13:17] * mhwood (~mhwood@mhw.ulib.iupui.edu) has joined #duraspace
[18:22] * misilot (~misilot@p-body.lib.fit.edu) Quit (Quit: Leaving)
[18:54] * misilot (~misilot@p-body.lib.fit.edu) has joined #duraspace
[18:56] * misilot (~misilot@p-body.lib.fit.edu) Quit (Remote host closed the connection)
[18:59] * misilot (~misilot@p-body.lib.fit.edu) has joined #duraspace
[19:54] <DSpaceSlackBot> <tdonohue> <here>: Reminder that our weekly DSpace DevMtg starts at the top of the hour here . Agenda at: https://wiki.duraspace.org/display/DSPACE/DevMtg+2017-10-11
[20:00] <DSpaceSlackBot> <tdonohue> Hi all, welcome! It's DevMtg time. As this is a late meeting (20UTC), I'd like to do a quick roll call of who is <here>. (Been finding lately that these late meetings are more sparsely attended)
[20:02] <DSpaceSlackBot> <terrywbrady> Im here
[20:02] <DSpaceSlackBot> <mwood> Hi
[20:02] <DSpaceSlackBot> <tdonohue> Ok, that's a small quorum then. We are 3 ;)
[20:04] <DSpaceSlackBot> <tdonohue> Quick usual reminders... DSpace 7 WG meeting is tomorrow at 15UTC (11am EDT). We also have a new DSpace Entities Working Group starting on Fri at 15UTC (11am EDT) to discuss future data model changes
[20:05] <DSpaceSlackBot> <tdonohue> The first two topics on the agenda are really intertwined. In the DSpace 7 WG discussions last week, the Angular team decided to adopt Coveralls to track unit test coverage: https://coveralls.io/github/DSpace/dspace-angular
[20:06] <DSpaceSlackBot> <tdonohue> The questions came up as to whether we should be doing similar automated code checks/management on `master` (for DSpace 7 REST API, etc)
[20:06] <DSpaceSlackBot> <tdonohue> And over the last week, there's been three ideas floated and/or begun... all listed under #2 in the agenda. I'd like feedback on these and whether the few of you here have thoughts/comments/objections, etc
[20:07] <DSpaceSlackBot> <terrywbrady> It would be interesting to see the report. Would it recognize the existence our our current test
[20:07] <DSpaceSlackBot> <tdonohue> @terrywbrady: we have that report ;)
[20:07] <DSpaceSlackBot> <mwood> Yes we should. But early results will be somewhat embarassing, I think.
[20:07] <DSpaceSlackBot> <tdonohue> Before we get to that report though, let me touch on 2.a.
[20:08] <DSpaceSlackBot> <tdonohue> Adding ErrorProne checks to `master`: https://github.com/DSpace/DSpace/pull/1861
[20:08] <DSpaceSlackBot> <tdonohue> This is a basic, build analysis tool (from Google) that checks Java code for common bugs/mistakes *at compile time*
[20:09] <DSpaceSlackBot> <tdonohue> I've create a PR for `master` (see above) to enable it...and fixed all the bugs it reports on `master` (there are not too many overall)
[20:09] <DSpaceSlackBot> <mwood> It seems to check for things that e.g. NetBeans doesn't. What little I've seen, seems sensible.
[20:10] <DSpaceSlackBot> <tdonohue> It doesn't check for a *lot* of things...but it caches some obvious bugs / bad code. So, I'd like to enable it by default...as it might catch obvious issues in future PRs, etc
[20:10] <DSpaceSlackBot> <mwood> It seems more aggressive toward situations where a reviewer might ask, "are you sure this is right?"
[20:11] <DSpaceSlackBot> <terrywbrady> It seems worthwhile to enable. If it introduces a problem, we could disable it it the future.
[20:11] <DSpaceSlackBot> <tdonohue> I'll also note that @hpottinger (who isn't here today) asked about possibly also enabling this for `dspace-6_x`. So, we might think about whether we'd want that too...but it'd likely be a separate PR (as ErrorProne might find more issues in XMLUI and/or JSPUI)
[20:12] <DSpaceSlackBot> <tdonohue> Ok, glad to hear you both approve. If one (or both) of you would approve the PR, we can get this merged ASAP then
[20:13] <DSpaceSlackBot> <mwood> It's +2 now.
[20:13] <DSpaceSlackBot> <tdonohue> Next in that list of code tools is *enabling code coverage checks on `master`*
[20:13] <DSpaceSlackBot> <tdonohue> @tom_desair has done some great work on this over the last few days, and he came up with two options
[20:13] <DSpaceSlackBot> <terrywbrady> merged
[20:14] <DSpaceSlackBot> <tdonohue> 1) Enabling Coveralls on `master` (similar to dspace-angular) via this PR: https://github.com/DSpace/DSpace/pull/1865 (Here's a report of current master coverage from Coveralls: https://github.com/DSpace/DSpace/commit/8e65cdad6d33a22125940e8129d13e6df858456c)
[20:14] <DSpaceSlackBot> <tdonohue> 2) Enabling SonarCloud on `master`... here's a similar report from SonarCloud: https://sonarcloud.io/dashboard?id=org.dspace%3Adspace-parent
[20:15] <DSpaceSlackBot> <tdonohue> Both show our coverage is really tiny (~18%)
[20:16] <DSpaceSlackBot> <tdonohue> Whoops. Correct Coveralls link is : https://coveralls.io/builds/13658718
[20:16] <DSpaceSlackBot> <terrywbrady> thanks for the edit... i was confused
[20:17] <DSpaceSlackBot> <tdonohue> The reports are similar...and the percentage calculated from each tool is roughly the same (~18%). SonarCloud provides a lot of extra detail about possible bugs ("code smells"), possible vulnerabilities, etc
[20:17] <DSpaceSlackBot> <tdonohue> While the info from SonarCloud is nice...I've found it to be a *lot* of false positives...and we'd likely need to highly tailor it to get better results
[20:18] <DSpaceSlackBot> <tdonohue> Coveralls, on the other hand is really just a report of code coverage, but it integrates tightly with Travis CI (allowing you to check all PRs for whether it improves code coverage or not, etc)
[20:19] <DSpaceSlackBot> <mwood> Keep it simple, I say.
[20:19] <DSpaceSlackBot> <tdonohue> So, the question here is twofold. (1) Would we like to see these sorts of reports automated for `master`? (Even if the numbers are a bit embarrassing right now)
[20:19] <DSpaceSlackBot> <tdonohue> (2) Do we have a preference for one of these tools?
[20:19] <DSpaceSlackBot> <mwood> Dunno about *like* but we probably *should*.
[20:19] <DSpaceSlackBot> <terrywbrady> I like what SonarCloud seems to promise... but I say that without knowing how noisy the false positives are.
[20:20] <DSpaceSlackBot> <terrywbrady> Would the Fedora (or other projects) folks have experience with either?
[20:20] <DSpaceSlackBot> <mwood> Maybe go with Coveralls for now, and take some time to play with Sonar. We can switch later if it's better.
[20:21] <DSpaceSlackBot> <tdonohue> Fedora does use SonarCube (which is the software behind SonarCloud): http://sonar.fcrepo.org/ But, they seem to have tightly tailored it to mostly do coverage reports
[20:21] <DSpaceSlackBot> <tdonohue> SonarQube...that is
[20:22] <DSpaceSlackBot> <terrywbrady> Did @tom_desair have a preference?
[20:22] <DSpaceSlackBot> <tdonohue> Coveralls is more simplistic, but I do really like how it integrates with Travis.. *plus* it works for more than just Java. For example, we are using Coveralls also for `dspace-angular` (Angular UI). So, we could use the same thing for both areas of codebase
[20:23] <DSpaceSlackBot> <terrywbrady> That seems like a good rationale for Coveralls
[20:23] <DSpaceSlackBot> <mwood> Rails folk seem to like Coveralls too.
[20:24] <DSpaceSlackBot> <tdonohue> @tom_desair said he had previous experience with SonarQube and liked it...but admitted it requires tailoring to get good results. (And I admit I'm worried about how much tailoring / time we'd have to spend to decrease on the false positives...a quick review shows a lot of false positives)
[20:24] <DSpaceSlackBot> <tdonohue> So, my personal gut feeling is to start with Coveralls
[20:25] <DSpaceSlackBot> <terrywbrady> :+1:
[20:25] <DSpaceSlackBot> <mwood> Aye, Coveralls.
[20:25] <DSpaceSlackBot> <tdonohue> Sounds good then. I'll help @tom_desair move that forward and we'll see how this goes. :slightly_smiling_face:
[20:26] <DSpaceSlackBot> <mwood> If someone wants to work on tailoring Sonar in the background, we could reconsider it for DSpace 8.
[20:27] <DSpaceSlackBot> <tdonohue> I will also admit, on the `dspace-angular` side we've configured Coveralls to throw an *error* (in the PR) if the PR decreases the code coverage by more than 1% (as it forces us to watch that closely and encourage code tests). I'd be tempted to do the same for `master` in the nearish future (with the understanding we can always override it for specific PRs, and/or turn it off if it gets annoying)
[20:28] <DSpaceSlackBot> <tdonohue> I'll be running that by the DSpace 7 team though too, so we'll see how they respond
[20:29] <DSpaceSlackBot> <tdonohue> Last in this list of "code mgmt tools" (2.c.)... *Checkstyle* While this is less about verifying good code, there is a `maven-checkstyle-plugin` (which Fedora and others use) which can validate the formatting of our Java code (e.g. where brackets sit, no spaces at ends of lines, etc)
[20:30] <DSpaceSlackBot> <tdonohue> I've been toying with the idea of possibly also looking at this for `master`. But, So far, I've not built out anything...just looked at Fedora's Checkstyle settings, and analyzed the Pros/Cons here
[20:30] <DSpaceSlackBot> <mwood> It would be nice to have a common style again.
[20:31] <DSpaceSlackBot> <tdonohue> The cool thing about Checkstyle is that you can configure it via a `checkstyle.xml` config file. Many major IDEs let you import that same config file (using a checkstyle plugin) to configure your IDE to use that style
[20:31] <DSpaceSlackBot> <terrywbrady> I find it frustrating to think I have finished a PR and then have feedback on style... if the checks can be automated it will be easier to comply
[20:31] <DSpaceSlackBot> <tdonohue> *If* we went with Checkstyle, we could then tell everyone to upload the checkstyle.xml config file into their IDE (Or give directions on pre-configuring their IDE) and then the IDE *should* prompt you to follow that style
[20:32] <DSpaceSlackBot> <mwood> We can likely write an XSL-T to format checkstyle.xml for humans, so we know what we are trying to do and commit fewer offenses to begin with.
[20:32] <DSpaceSlackBot> <terrywbrady> Lets do it... I presume that there would be a coordinated mega PR to bring style into conformance.
[20:33] <DSpaceSlackBot> <tdonohue> @mwood: yes, either that, or have a better guide. Fedora documents this more clearly here: https://wiki.duraspace.org/display/FF/Code+Style+Guide And then they turn those instructions into code via their Checkstyle config
[20:33] <DSpaceSlackBot> <mwood> IIRC our style document was a configuration file for Eclipse.
[20:34] <DSpaceSlackBot> <mwood> A lot of newcomers are likely unaware of it.
[20:34] <DSpaceSlackBot> <tdonohue> @terrywbrady: yes, that's why I'm floating this idea first. It would require a "mega-PR" to bring all current code into "compliance". So, we'd have to coordinate the building/merger of that mega PR
[20:35] <DSpaceSlackBot> <tdonohue> It also is an opportunity to review the *old* style docs (for Eclipse), review Fedora's guide (or other third-party guides) and determine what we feel is a the best style for DSpace *now* (well for DSpace 7 and future)
[20:35] <DSpaceSlackBot> <terrywbrady> It would be nice to alert folks that they might want to hold off on PR submission while the big PR is being applied.
[20:35] <DSpaceSlackBot> <tdonohue> Google also has a very detailed Java style guide (we may not want to get this detailed though): https://google.github.io/styleguide/javaguide.html
[20:36] <DSpaceSlackBot> <terrywbrady> Which branches are you thinking for style conformance? All 3 active branches + master?
[20:37] <DSpaceSlackBot> <tdonohue> I was actually thinking of limiting this to `master` to avoid major chaos with all open PRs. That way it'd only affect folks working on DSpace 7 (and or porting PRs to DSpace 7)
[20:37] <DSpaceSlackBot> <tdonohue> It also makes it easier to do it only on `master` cause then you don't need to deal with bringing JSPUI and/or XMLUI into "compliance"
[20:38] <DSpaceSlackBot> <tdonohue> The downside is that fixes that need to be applied to `dspace-6_x` AND `master` may need separate PRs, as cherry-picking may not work right
[20:38] <DSpaceSlackBot> <terrywbrady> That seems reasonable although it will make cherry picking more difficult as you have suggested
[20:39] <DSpaceSlackBot> <mwood> Why would 6x, which is not enforcing style, care?
[20:39] <DSpaceSlackBot> <terrywbrady> 6x will be production for most folks for 2-3 years, so I presume that there will be a lot of fixes that originate there and need to be ported to master.
[20:40] <DSpaceSlackBot> <tdonohue> I don't know if I understand that question @mwood. 6.x shouldn't care if we only apply the style changes to `master`. But, it cherry-picking may not work between those branches
[20:40] <DSpaceSlackBot> <mwood> Depends on direction, I guess.
[20:41] <DSpaceSlackBot> <tdonohue> I doubt cherry-picking will work well in either direction...as in some cases brackets may get moved around, etc...so some classes may have more or less lines, even if the code is "identical" on `master` and 6.x
[20:42] <DSpaceSlackBot> <tdonohue> So, I think we'd have to admit we'd be creating a bit more work when it comes to cherry-picking changes..but, maybe that's OK and we just start creating multiple PRs if a change needs to be on 6.x and master
[20:42] <DSpaceSlackBot> <mwood> Is Checkstyle keeping up with Java 8/9 new stuff? Will it say "huh?" if I write try-with-resources or lambdas or whatever?
[20:43] <DSpaceSlackBot> <terrywbrady> I do not know if this is a good idea, but one could selectively make modules (dspace-api) within dspace-6_x conform to the new style guides.
[20:43] <DSpaceSlackBot> <tdonohue> @mwood: yes. Checkstyle also lets you tweak which things to enforce. You can tell it you want to require try-with-resources...or you can tell it to ignore that check (in which case it "won't care").
[20:44] <DSpaceSlackBot> <mwood> I only ask that it understand.
[20:44] <DSpaceSlackBot> <tdonohue> So, in saying we want to use Checkstyle we'd also have to decide what we feel we would like to enforce.
[20:44] <DSpaceSlackBot> <tdonohue> Here's the Checkstyle main codebase...it's very well maintained: https://github.com/checkstyle/checkstyle
[20:45] <DSpaceSlackBot> <tdonohue> And the `maven-checkstyle-plugin` simply pulls in the version of Checkstyle you ask for, and then runs the enforcement during the Maven build process
[20:46] <DSpaceSlackBot> <terrywbrady> I recommend turning it on for master and then raise the question about dspace-6_x, etc in a couple months.
[20:47] <DSpaceSlackBot> <mwood> That sounds well.
[20:47] <DSpaceSlackBot> <tdonohue> So, it sounds here like both of you approve of enabling Checkstyle, and approve of enabling it on `master` only? That'd mean the next steps are to bring this to the DSpace 7 team (tomorrow), and then start a Wiki page to help us (re)define our "DSpace Style rules" (and review what we had defined previously for Eclipse)
[20:48] <DSpaceSlackBot> <terrywbrady> And re-visit the question of Checkstyle for the other branches in a couple of months.
[20:48] <DSpaceSlackBot> <tdonohue> I can take the lead on the above :point_up:, and report back on this, etc. Obviously this won't happen immediately, but I'd like to get the process started, so that we can start to document it, determine when would be a good time to add this into `master` (and warn everyone), etc.
[20:49] <DSpaceSlackBot> <tdonohue> And I like @terrywbrady's idea to start on `master` and then look towards other branches in the future.
[20:49] <DSpaceSlackBot> <mwood> Maybe start out by not failing the build if Checkstyle is unhappy, until we have time to clean things up.
[20:50] <DSpaceSlackBot> <tdonohue> @mwood: yes, possibly. Though I worry that kicks the problem down the road (with regards to cleaning things up). We already see PRs to DSpace 7 REST API with different ideas of formatting/style
[20:51] <DSpaceSlackBot> <tdonohue> So, for DSpace 7 development purposes, it might be good to have this in sooner rather than later. But, that said, I don't want to rush it in. Just want to get the discussion started so we can plan it out
[20:52] <DSpaceSlackBot> <tdonohue> In any case, I'm glad you both agree here. I wasn't sure if everyone would like this idea, but I feel this is a good time to possibly make this step (with DSpace 7 work)
[20:52] <DSpaceSlackBot> <mwood> I worry that it will be a huge job just to get master aligned with the rules. Maybe we should enable failure on one project at a time?
[20:52] <DSpaceSlackBot> <mwood> Start with spring-rest.
[20:53] <DSpaceSlackBot> <tdonohue> That could work, yes, to do it project by project. "compliance" though really depends on what the rules defined are (so that's the first step here)
[20:54] <DSpaceSlackBot> <mwood> Yes, first we need a style we all hate equally.
[20:55] <DSpaceSlackBot> <tdonohue> I think some of the most problematic code in terms of style was in XMLUI...and as that is gone, I'm hopeful that the API itself isn't too horribly bad. But, yes, this would require touching a lot of files...so breaking it up per project may be best
[20:56] <DSpaceSlackBot> <tdonohue> Ok, realizing we are nearly out of time. This has been a good discussion here, and glad to see we all like the movement towards these automated checks/tools.
[20:56] <DSpaceSlackBot> <terrywbrady> Do any tools exist that can "prove" that style changes made to code have no meaningful impact on the generated byte code?
[20:56] <DSpaceSlackBot> <mwood> I'm hoping that something will help us straighten out tabs vs. spaces vs. mixed tabs-and-spaces. :-P
[20:57] <DSpaceSlackBot> <tdonohue> One other thing I want to quickly mention is @tom_desair's work on a REST API Test Framework here: https://github.com/DSpace/DSpace/pull/1866 (I mention it cause it could use feedback from others, not just DSpace 7 developers)
[20:57] <DSpaceSlackBot> <terrywbrady> I think we are going to wish for that on 6x !
[20:58] <DSpaceSlackBot> <tdonohue> @terrywbrady: pretty sure that is proven, but I have no tools to point at here. I do know it is very common to enforce Java code style rules in Java projects. We just have never done so...but now that our Java code is decreased slightly (with no more JSPUI/XMLUI on master), it seems an opportunity to do so
[20:59] <DSpaceSlackBot> <terrywbrady> I want to make a pitch for someone to work through this tutorial: https://github.com/terrywbrady/restReportTutorial/blob/master/README.md . I think it will be a fun exercise AND it is an opportunity to knock out 3 simple PR's at one time.
[21:00] <DSpaceSlackBot> <mwood> Keep asking! I want to learn to use those tools....
[21:01] <DSpaceSlackBot> <tdonohue> I'm not sure I'll have time myself this wek, @terrywbrady ... but that tutorial does look excellent. Maybe something to ping folks on in dev or dspace-devel to see if we can get some reviewers/testers to offer feedback?
[21:02] <DSpaceSlackBot> <tdonohue> If anyone has time to review PR#1866 (REST API Test Framework) :point_up: that could also use eyes/comments/thoughts
[21:02] <DSpaceSlackBot> <terrywbrady> I will make another pitch.
[21:02] <DSpaceSlackBot> <mwood> I *know* that we have some dirty metadata....
[21:03] <DSpaceSlackBot> <terrywbrady> If this tutorial approach looks useful, I think we could build other similar modules such as "How to create a custom facet" or "How to customize and item page".
[21:03] <DSpaceSlackBot> <mwood> I think those would be popular.
[21:05] <DSpaceSlackBot> <tdonohue> I agree, they likely would be very popular. We also may want to consider how to promote them better (make them findable)...whether we link from official docs, fork over in `DSpace-Labs` repo, or a bit of both?
[21:05] <DSpaceSlackBot> <terrywbrady> I considered DSpace-Labs for the repo. If folks find this useful, I can port the repo there.
[21:05] <DSpaceSlackBot> <tdonohue> So, @terrywbrady if you have time / inclination, I like this direction. We'll just have to think about promoting it so that folks can easily find your work
[21:06] <DSpaceSlackBot> <tdonohue> The one advantage of DSpace-Labs is that we can easily promote others to work on a "module" to enhance it, etc. We can give commit rights per project ;)
[21:06] <DSpaceSlackBot> <mwood> Thank you for making that tutorial available!
[21:07] <DSpaceSlackBot> <tdonohue> As we are now "over time" here, I'm not going to bring up other topics. Unfortunately, I also have to depart shortly, but feel free to hang around as much as you wish
[21:07] <DSpaceSlackBot> <mwood> I have to run, as well.
[21:09] * mhwood (~mhwood@mhw.ulib.iupui.edu) Quit (Remote host closed the connection)
[21:09] <DSpaceSlackBot> <tdonohue> Thanks all!
[21:44] * tdonohue (~tdonohue@dspace/tdonohue) Quit (Read error: Connection reset by peer)
These logs were automatically created by DuraLogBot on irc.freenode.net using the Java IRC LogBot.