-
Notifications
You must be signed in to change notification settings - Fork 2
states instead of files #34
Conversation
@h1alexbel thank you for your Pull Request. I'll assign someone to review it soon. If this PR solves a |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #34 +/- ##
=============================================
+ Coverage 30.50% 52.50% +21.99%
- Complexity 8 11 +3
=============================================
Files 6 7 +1
Lines 59 80 +21
Branches 0 1 +1
=============================================
+ Hits 18 42 +24
+ Misses 41 38 -3
☔ View full report in Codecov by Sentry. |
@h1alexbel please review this Pull Request. Deadline (when it should be merged or closed) is You should check if the requirements have been implemented (partially or in full), if there are unit tests covering the changes and if the CI build passes. Feel free to reject the PR or ask for changes if it's too big or not clear enough. Estimation here is |
@zoeself deregister |
@h1alexbel ok, I've removed this task from scope. I'm not managing it anymore. |
@zoeself register |
@h1alexbel I've just registered this ticket as a task and will assign it to someone soon. Thanks! |
@h1alexbel please review this Pull Request. Deadline (when it should be merged or closed) is You should check if the requirements have been implemented (partially or in full), if there are unit tests covering the changes and if the CI build passes. Feel free to reject the PR or ask for changes if it's too big or not clear enough. Estimation here is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h1alexbel take a look at my comments below, please
value, | ||
new IsEqual<>( | ||
new ListOf<>( | ||
"CREATE KEYSPACE queryDatasets\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in java 17 we can use """
as far as i remember
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@l3r8yJ yeah, but JTCOP doesn't support it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h1alexbel i think it's reason to suppress it and create a bug report, preferably with a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@l3r8yJ we can't suppres it, it will fail on Java version, bug report idea looks good
@l3r8yJ master code a bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h1alexbel much better now, take a look again, please
value, | ||
new IsEqual<>( | ||
new ListOf<>( | ||
"CREATE KEYSPACE queryDatasets\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about """
and bug report to jtcop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import java.util.List; | ||
|
||
/** | ||
* Test suite for {@link StateChanges}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Test suite for {@link StateChanges}. | |
* Test case for {@link StateChanges}. |
I think that name might confuse some programmers which familiar with TestNG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@l3r8yJ test suite is a set of test cases;
test suite in java presented test class
So, either Test suite or Test cases, but not a Test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@h1alexbel looks good to me!!
@rultor merge |
@h1alexbel OK, I'll try to merge now. You can check the progress of the merge here |
@h1alexbel Done! FYI, the full log is here (took me 2min) |
@h1alexbel thank you for resolving this ticket. I've just added it to your active invoice. You can always check all your invoices and more on the Contributor Dashboard. |
closes #32
@l3r8yJ take a look, please
PR-Codex overview
Detailed summary
master.xml
file to include states and change states.State
interface inState.java
.Ids
class toSha
inSha.java
.Names
andAuthors
classes to retrieve file names and authors based on state ID.Author
,Sha
, andStateChanges
.Master
class to use newNames
constructor.