Skip to content

Feature/LOC #1987

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

Merged
merged 7 commits into from
Feb 12, 2018
Merged

Feature/LOC #1987

merged 7 commits into from
Feb 12, 2018

Conversation

idodeclare
Copy link
Contributor

Hello,

Please consider for integration this patch to count per-file physical lines-of-code (LOC;
Wiki) and to store LOC and total number of lines.

DirectoryListing is updated to present the counts while browsing.

Thank you.

@idodeclare
Copy link
Contributor Author

I should note that this resolved issue #591.

@tulinkry
Copy link
Contributor

I would personally prefer if all the methods mentioning LOC would have it expanded to LinesOfCode.

@@ -272,7 +274,7 @@ public void run() {

@SuppressWarnings("PMD.CollapsibleIfStatements")
private void initialize() throws IOException {
synchronized (this) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this make any difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just that synchronizing on public objects is suspect as it breaks encapsulation — so I revised it.

@@ -805,7 +805,7 @@ private boolean accept(File file) {
return !RuntimeEnvironment.getInstance().isIndexVersionedFilesOnly();
}

boolean accept(File parent, File file) {
private boolean accept(File parent, File file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this relevant? or just a cleanup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a cleanup

@idodeclare
Copy link
Contributor Author

I would personally prefer if all the methods mentioning LOC would have it expanded to LinesOfCode.

I was motivated to have it quite terse for the methods that are used in analyzers, since I had to edit hundreds of lines in them. Moreover since LOC is almost orthogonal to the bulk of logic in the xrefers, I wanted a tiny function name — chkLOC() — rather than seeing checkLinesOfCode() over and over (and also having to reformat possibly hundreds of newly too-long lines).

I do think "LOC" is well-known enough acronym in source code analysis; if analyzers are ever updated to support logical lines-of-code, then "LLOC" also will be well-known.

@ChristopheBordieu
Copy link

Excellent feature!!!
Will all the total lines and slocs be aggregated at repository root level ?

@vladak
Copy link
Member

vladak commented Jan 26, 2018

@idodeclare pls use 'fixes #591' in one the commit comments then.

@idodeclare
Copy link
Contributor Author

Will all the total lines and slocs be aggregated at repository root level ?

That is certainly feasible in the future. Right now, only files have document data in the Lucene index. To store aggregated num-lines and LOC, it will be necessary to store Lucene data for directories — but without conflicting with the existing queries that all assume only file-level data.

One straight-forward idea would be to add Lucene "documents" for directories with wholly-separate fields so that these new documents never appear in current queries. Of course it would also be necessary to write an aggregator that runs at the appropriate stages.

@vladak
Copy link
Member

vladak commented Jan 26, 2018 via email

@idodeclare
Copy link
Contributor Author

This also brings a question about how to use these counts for restricting
searches.

Yes, indeed.

Relatedly, I did not update to show the counts in search results, as I did not have any idea how best to present them.

@tarzanek tarzanek added this to the 1.1 milestone Feb 5, 2018
@tarzanek
Copy link
Contributor

I am merging this as is, thank you Chris, it looks good

@tarzanek tarzanek merged commit a7351f5 into oracle:master Feb 12, 2018
@idodeclare idodeclare deleted the feature/loc branch February 12, 2018 15:00
@idodeclare
Copy link
Contributor Author

Thanks, @tarzanek !

@vladak
Copy link
Member

vladak commented Feb 13, 2018

I wonder how this will cope with incremental indexing. Will pre-existing files have LOC set to 0 ? Or is full reindex required ?

@idodeclare
Copy link
Contributor Author

Pre-existing documents will have blank LOC/#Lines — the same as for files which do not get an xref (e.g. Ignored/FileAnalyzer) or for files that do not have a counting xref (e.g. UuencodeAnalyzer).

Incrementally re-indexed files can get LOC/#Lines. Full re-index is required for completeness.

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.

5 participants