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

HSEARCH-3478 + HSEARCH-3425 Code cleanup #1879

Merged
merged 56 commits into from Feb 8, 2019

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Feb 7, 2019

https://hibernate.atlassian.net//browse/HSEARCH-3425
https://hibernate.atlassian.net//browse/HSEARCH-3478

Note I don't expect a full review, it would be quite boring.

@gsmet, could you simply double-check that the warnings are gone when you use Eclipse? They are gone in my instance of Eclipse, but who knows.
Of course you need to ignore the legacy modules; personally I moved the non-legacy modules to a separate working set, I configured the "Problems" view to only show problems in the selected projects, and I selected the "non-legacy" working set.

... the rule also accepts switches where we define a case for each enum
constants.
Some warnings allowed me to find out they were not covered by tests.
…ntainer extractors

Spotted thanks to warnings about BuiltinContainers.OPTIONAL_LONG not
being used.
@yrodiere
Copy link
Member Author

yrodiere commented Feb 7, 2019

Note for future reference.

IDEA doesn't offer a global view of all warnings in the codebase, making it difficult to keep the number of warnings to 0. The feature was requested a long time ago and never implemented, so it's unlikely it will ever be: https://youtrack.jetbrains.com/issue/IDEA-68854

Thus I wanted to find some solution to be aware of added warnings and fix them as I code.

I tried to fail the build on warnings by passing -Werror to the compiler, but it seems to stop after the very first warning. Clearly not good.

I also tried to make all warnings appear in IDEA in the "Messages" view, shown when we trigger a full build. That's better than nothing, I thought. Well, no: by passing -Xlint:all to the compiler for non-legacy modules, and -Xlint:none -nowarn for legacy modules, and IDEA correctly detected the options (they appear in the build settings). But for some reason, a lot of warning still appear even for legacy modules. Also, the output seems to include irrelevant warnings, such as warnings we suppressed using @SuppressWarnings, or heap pollution warnings even though we added @SafeVarargs. So we can't control which warnings appear, and there are way too many warnings. Useless.

I'm not going to go back to Eclipse, because even without a global view of warnings, IDEA is still better. And more importantly I won't force people working on Hibernate Search to use one IDE or another.

So... I'm out of idea. I guess we'll have to periodically clean up the codebase...

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1

  • 194 of 208 (93.27%) changed or added relevant lines in 103 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.5%) to 87.904%

Changes Missing Coverage Covered Lines Changed/Added Lines %
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/client/impl/ElasticsearchClientFactoryImpl.java 1 2 50.0%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/index/admin/gson/impl/AnalysisJsonElementEquivalence.java 0 1 0.0%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/index/admin/impl/ElasticsearchSchemaValidatorImpl.java 2 3 66.67%
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/orchestration/impl/ElasticsearchSerialChangesetsWorkOrchestrator.java 0 1 0.0%
backend/lucene/src/main/java/org/hibernate/search/backend/lucene/util/impl/AnalyzerUtils.java 8 9 88.89%
engine/src/main/java/org/hibernate/search/engine/common/impl/ErrorContextImpl.java 0 1 0.0%
engine/src/main/java/org/hibernate/search/engine/logging/spi/MappingKeyFormatter.java 0 1 0.0%
engine/src/main/java/org/hibernate/search/engine/spatial/ImmutableGeoPolygon.java 0 1 0.0%
mapper/orm/src/main/java/org/hibernate/search/mapper/orm/cfg/HibernateOrmIndexingStrategyName.java 2 3 66.67%
mapper/orm/src/main/java/org/hibernate/search/mapper/orm/massindexing/impl/MassIndexerImpl.java 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/gson/impl/UnexpectedJsonElementTypeException.java 1 0.0%
mapper/orm/src/main/java/org/hibernate/search/mapper/orm/bootstrap/impl/HibernateSearchIntegrator.java 1 94.23%
Totals Coverage Status
Change from base Build 136: 0.5%
Covered Lines: 14541
Relevant Lines: 16542

💛 - Coveralls

@yrodiere yrodiere requested a review from gsmet February 8, 2019 07:19
@yrodiere yrodiere merged commit a5b9475 into hibernate:master Feb 8, 2019
@yrodiere
Copy link
Member Author

yrodiere commented Feb 8, 2019

Approved over the chat. Merged, thanks!

@yrodiere yrodiere deleted the HSEARCH-3425 branch February 13, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants