You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jena.apache.org by ajs6f <gi...@git.apache.org> on 2015/05/08 16:11:40 UTC
[GitHub] jena pull request: Kill dead things
GitHub user ajs6f opened a pull request:
https://github.com/apache/jena/pull/58
Kill dead things
NOT FOR MERGING.
This PR is just a resource for selecting smaller, more carefully-examined packages of changes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ajs6f/jena KillDeadThings
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/jena/pull/58.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #58
----
commit fc0fe86b5ae9ab7b53885c81f00fe55f90421bbb
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-07T23:43:31Z
Removing code that is demonstrably impossible to execute, based on the calculations of Eclipse and javac.
commit 348152a177331b5a46536abfc4657254762929b0
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-08T00:17:21Z
Removing unnecessary superinterfaces, unused local variables, unthrown exceptions.
commit 2fef2f5567a227e2701dc1eb7db8bc64dc4d6f01
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-08T01:40:17Z
Further removal of dead code
commit 058c3e4074847b71b52304c54d8484203583d8b6
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-08T01:46:41Z
Further removal of dead code in jena-text
commit da89f84d0a4440129301bf922235a9742a3204a3
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-08T02:23:38Z
Removing unused fields
commit 82aade301dd764fd73f0ea9a94c9f73857f88a32
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-08T02:56:39Z
More unused fields
commit 7a468e7d529cb94603de948dc34b26704d249e05
Author: ajs6f <aj...@virginia.edu>
Date: 2015-05-08T13:41:42Z
Removing unthrown checked exceptions
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/58#issuecomment-101273013
I don't think you owe me any apologies. It's just a case of a new guy showing up to an open source project who has to spend some time getting to know the rhythm before joining the dance.
It definitely sounds like some effort at "codification" (in the lightest and gentlest sense) would better repay than more wrangling on the code itself. It might benefit other latecomers, and if it seems to be getting bogged down in minutia or dogmatism, we can just cut it short and take forward whatever _was_ learnt.
I'll shoot something to dev@ today. Here is a strawman:
It would be nice to have some rough code standards that would help newcomers without inhibiting anyone from contributing. Possible simple policies include:
- Don't throw checked exceptions that aren't really ever thrown unless an API supertype specs it.
- Don't leave unused imports in. Any IDE can solve that problem for you with one keystroke.
- If a type declares a supertype that isn't a required declaration, consider whether that clarifies or confuses the intent. The former is okay, the latter not so good.
- Minimize the compiler warnings your code throws up. If you use `@SuppressWarnings` to hide them, please add a comment explaining the situation or a TODO with a potential future fix that would allow removing the suppression.
- Please remove unused local variables or fields or uninteresting private methods. If it's debugging detritus, consider adding formal logging code for future use, if it seems likely to be useful.
- If there is valuable code in some unused private method, add a `@SuppressWarnings` with an explanation of when it might become useful. If there is valuable but "dead" code inside a "live" method, consider breaking it out into a private method and adding `@SuppressWarnings` and explanation.
After discussion on the list and assuming that goes well, I'll take it to the CMS and to a ticket to start getting some feedback from Checkstyles from actual processing of the code. If Jena has some public CI, it would be great to get something like [Sonar](http://www.sonarqube.org/) engaged there for telemetrics.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by afs <gi...@git.apache.org>.
Github user afs commented on the pull request:
https://github.com/apache/jena/pull/58#issuecomment-101256776
Good point. I wasn't able to catch on this PR before the JENA-938 ones emerged over the weekend so you have got ahead of me and everyone else. My apologies.
Finding time has to be balanced with $dayjob and life.
The project has not traditionally had such policies decided upfront, they emerge. Now may be a good time to capture them now they have emerged. I am not keen on defining such things too early because such discussions focus on the controversial points yet they c an be a small, important part of the whole. Instead, having concrete code to talk about means focusing on what has the most benefit.
Would you like to draft a proposal and email dev@? And then progress to adding some pages to the website.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by ajs6f <gi...@git.apache.org>.
Github user ajs6f commented on the pull request:
https://github.com/apache/jena/pull/58#issuecomment-101236372
This is all good info, although I feel a bit unfortunate for having done these "speculative" PRs to get it. Can some kind of policy be published for these rules-of-thumb? Even better, if there is committer consensus around them, perhaps they can be "enshrined" in Checkstyles rules to give potential contributors some tactile feedback?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/jena/pull/58
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by afs <gi...@git.apache.org>.
Github user afs commented on the pull request:
https://github.com/apache/jena/pull/58#issuecomment-101213856
JENA-938
(using this PR to cover #60, #61, #62, #63)
Some of these are style points:
Unthrown checked exceptions: many (not all but most in my sampling) of these are where the interface declaration has an exception but the code implementation does not throw it currently.
Autogenerated code from an interface will include the exception declaration and it is a right to have a later change and then throw the exception. This will have an effect if the app code is using the implementing class, so when an interface is part, not all, of a classes contract.
Unnecessary superinterface declarations: this is style - if inheriting from a base class. it can be clearer to keep the "implement interface".
Needless typecasts: probably a left-over from converting to generics and should be cleared. Or int/long conversion which also can be cleared up.
In all cases, a principle of "keep even if redundant to be clear" needs to apply. (For me, the int/long cast conversion can fall into that category - can't remember the rules at the time and probably won't next time I see the code and certainly do not want accidiently truncation!)
Unused imports: my personal preference is no unused imports.
#58 also mentions unused variables (almost always should go IMO), unused fields (almost always should go, occasional they are internal trackers like stats.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by afs <gi...@git.apache.org>.
Github user afs commented on the pull request:
https://github.com/apache/jena/pull/58#issuecomment-103970036
Changes based the ideas here have been applied to the codebase (See JENA-938).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] jena pull request: Kill dead things
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the pull request:
https://github.com/apache/jena/pull/58#issuecomment-101428998
@ajs6f we've requested SonarQube for Jena about three weeks ago https://issues.apache.org/jira/servicedesk/customer/portal/1/INFRA-9469 Waiting for infra :smile:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---