Skip to content

Conversation

avgprog
Copy link
Contributor

@avgprog avgprog commented Feb 22, 2021

Resolves #246. Changes made:

  • Added deprecated tag to javadoc of FileNamingStrategy.java
  • Created EntityPersistenceNamingStrategy.java as a copy of FileNamingStrategy.java for replacing FileNamingStrategy.java
  • Refactored the code by changing the order of the functions for better readability

…rategy for replacing it and refactored the code
@avgprog avgprog changed the title Added deprecated tag to javadoc of FileNamingStrategy and created EntityPersistenceNamingStrategy for replacing FileNamingStrategy Created EntityPersistenceNamingStrategy for replacing FileNamingStrategy Feb 22, 2021
@avgprog avgprog self-assigned this Feb 22, 2021
@avgprog
Copy link
Contributor Author

avgprog commented Mar 10, 2021

  • Duplicated test cases of FileNamingStrategy to test EntityPersistenceNamingStrategy
  • Replaced occurences of FileNamingStrategy with EntityPersistenceNamingStrategy
  • Updated the javadocs to reflect the changes in the class name and its function
  • Moved EntityPersistenceNamingStrategy.java and HierarchicFileNamingStrategy.java to package edu.ie3.datamodel.io.naming in the src\main\java folder and moved EntityPersistenceNamingStrategyTest.groovy and HierarchicFileNamingStrategyTest.groovy to package edu.ie3.datamodel.io.naming in the src\test\groovy folder
  • Merged the latest code with the branch and resolved the conflicts

@johanneshiry
Copy link
Member

!test

@avgprog
Copy link
Contributor Author

avgprog commented Mar 10, 2021

Sonarqube shows several warnings with respect to the regex patterns declared in EntityPersistenceNamingStrategy.java: 'Use the named groups of this regex or remove the names.'. Also, some Codacy Static Code Analysis issues were with respect to the size of some functions in EntityPersistenceNamingStrategyTest.groovy. Should I work on resolving these issues?

@ckittl
Copy link
Member

ckittl commented Mar 11, 2021

Sonarqube shows several warnings with respect to the regex patterns declared in EntityPersistenceNamingStrategy.java: 'Use the named groups of this regex or remove the names.'. Also, some Codacy Static Code Analysis issues were with respect to the size of some functions in EntityPersistenceNamingStrategyTest.groovy. Should I work on resolving these issues?

The code quality of tests in detail doesn't really matter to me. But if there is something in production code, we should try to fix it (cf. "camp site rule" or "scout's rule" of clean code).

@avgprog
Copy link
Contributor Author

avgprog commented Mar 22, 2021

I have fixed the errors pointed out by Codacy and I tried fixing the issue pointed out by Sonarqube which says that there are unused named groups in the regex patterns in the file EntityPersistenceNamingStrategy.java. To resolve the issue, I removed the function call to get the member variable with the member variable itself (since the call was being made inside the class of which the variable is a member variable). The issue still exists according to Sonarqube, although, clearly, the named groups are actually being used in the same file. These named groups cannot be removed too as Sonarqube points out because they are indeed being used.

@avgprog
Copy link
Contributor Author

avgprog commented Mar 22, 2021

Quality gate test, however, is failing because I guess there is duplicate code as we have both FileNamingStrategy.java and EntityPersistenceNamingStrategy.java and their respective test case files in the code. So, I will try deleting the deprecated files.

@avgprog
Copy link
Contributor Author

avgprog commented Mar 22, 2021

This commit can be reverted if the files were not supposed to be deleted. All checks are passing so I am guessing the duplicate code was the issue which was resolved after deleting the files: FileNamingStrategy.java and FileNamingStrategyTest.groovy.

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #279 (4f60799) into dev (eed20af) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 4f60799 differs from pull request most recent head a33889c. Consider uploading reports for the commit a33889c to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##                dev     #279   +/-   ##
=========================================
  Coverage     77.98%   77.99%           
- Complexity     1951     1952    +1     
=========================================
  Files           249      249           
  Lines          7936     7939    +3     
  Branches        749      750    +1     
=========================================
+ Hits           6189     6192    +3     
  Misses         1321     1321           
  Partials        426      426           
Impacted Files Coverage Δ Complexity Δ
...edu/ie3/datamodel/io/source/csv/CsvDataSource.java 90.05% <0.00%> (-0.06%) 53.00% <0.00%> (-1.00%)
...in/java/edu/ie3/datamodel/io/sink/CsvFileSink.java 74.39% <0.00%> (ø) 29.00% <0.00%> (ø%)
...n/java/edu/ie3/datamodel/io/sink/InfluxDbSink.java 79.27% <0.00%> (ø) 24.00% <0.00%> (ø%)
...edu/ie3/datamodel/io/source/csv/CsvTypeSource.java 100.00% <0.00%> (ø) 13.00% <0.00%> (ø%)
.../ie3/datamodel/io/connectors/CsvFileConnector.java 77.38% <0.00%> (ø) 33.00% <0.00%> (ø%)
.../ie3/datamodel/io/source/csv/CsvWeatherSource.java 80.26% <0.00%> (ø) 20.00% <0.00%> (ø%)
...3/datamodel/io/source/csv/CsvTimeSeriesSource.java 96.07% <0.00%> (ø) 14.00% <0.00%> (ø%)
...odel/io/source/csv/CsvTimeSeriesMappingSource.java 100.00% <0.00%> (ø) 4.00% <0.00%> (ø%)
.../models/voltagelevels/GermanVoltageLevelUtils.java 81.25% <0.00%> (ø) 3.00% <0.00%> (ø%)
...datamodel/io/csv/HierarchicFileNamingStrategy.java
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40c3ed9...a33889c. Read the comment docs.

@avgprog avgprog marked this pull request as ready for review March 22, 2021 13:18
@avgprog avgprog requested a review from johanneshiry March 22, 2021 13:19
Copy link
Member

@ckittl ckittl left a comment

Choose a reason for hiding this comment

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

Looks good by around 90 % to me. Please try and improve the comments, they are mainly regarding documentation issues.

ckittl
ckittl previously approved these changes Mar 31, 2021
@ckittl ckittl merged commit 68447b2 into dev Mar 31, 2021
@ckittl ckittl deleted the sb/#246-replacingFileNamingStrategy branch March 31, 2021 07:56
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.

Rename and move FileNamingStrategy
3 participants