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

#1022 TempFolder #1049

Merged
merged 5 commits into from
Feb 9, 2019
Merged

#1022 TempFolder #1049

merged 5 commits into from
Feb 9, 2019

Conversation

BinaryIgor
Copy link
Contributor

As described in #1022. New class for creating temporary directories was created. It is very similar to TempFile, but it can create directories only.

@0crat 0crat added the scope label Jan 29, 2019
@0crat
Copy link
Collaborator

0crat commented Jan 29, 2019

Job #1049 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Jan 29, 2019

This pull request #1049 is assigned to @fabriciofx/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

Copy link
Contributor

@fabriciofx fabriciofx left a comment

Choose a reason for hiding this comment

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

@Iprogrammerr Done. Please, check it out.

* closed.
* @since 1.0
*/
public final class TempFolder implements Scalar<Path>, Closeable {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Iprogrammerr Please, create a new secondary ctor which create a TempFolder with a random name by default. This name should start with tmp prefix and more 5 random chars (it sholdn't exceed 8 chars size).

* @param path Relative path to new directory.
* @since 1.0
*/
public TempFolder(final String path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Iprogrammerr This ctor should be final Text path.

private final Scalar<Path> folder;

/**
* Ctor.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Iprogrammerr Create a new secondary ctor which signature should be TempFolder(final String path).

@BinaryIgor
Copy link
Contributor Author

@fabriciofx Done.

new TextOf(""),
new TextOf("tmp"),
// @checkstyle MagicNumber (1 line)
new RandomText(5, 'a', 'b', 'c')
Copy link
Contributor

Choose a reason for hiding this comment

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

@Iprogrammerr RandomText should cover [a-zA-Z0-9].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabriciofx How to do that? RandomText does not accept regex. You want me to create ListOf with 62 elements given in a ctor?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Iprogrammerr I said [a-zA-Z0-9] just to simplify. The idea is use a...z, A...Z and 0...9. What's the problem with a ctor with 62 elements? Look inside RamdomText. It has a ctor with more than 62 elements (I just don't say to use this ctor because it has special characters too).

@BinaryIgor
Copy link
Contributor Author

@fabriciofx Refactored.

@fabriciofx
Copy link
Contributor

@Iprogrammerr This PR seems fine to me, but due the gitlint errors I don't know if @llorllale will aproves it. @llorllale WDYT?

@fabriciofx
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 1, 2019

@rultor merge

@fabriciofx Thanks for your request. @llorllale Please confirm this.

@llorllale
Copy link
Contributor

@Iprogrammerr pull from master (don't rebase) in order to retrigger the build

@BinaryIgor
Copy link
Contributor Author

@llorllale I don't understand, how pulling is supposed to do that? It will trigger nothing. Didn't you mean merge?

@llorllale
Copy link
Contributor

@Iprogrammerr I just meant to update your PR with master without rebasing

@BinaryIgor
Copy link
Contributor Author

@llorllale Done.

@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #1049 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1049      +/-   ##
============================================
+ Coverage     87.58%   87.65%   +0.07%     
- Complexity     1520     1527       +7     
============================================
  Files           267      268       +1     
  Lines          3897     3922      +25     
  Branches        215      215              
============================================
+ Hits           3413     3438      +25     
  Misses          435      435              
  Partials         49       49
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/io/TempFolder.java 100% <100%> (ø) 7 <7> (?)

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 773f8e5...66a9e1f. Read the comment docs.

@llorllale
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 9, 2019

@rultor merge

@llorllale OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 66a9e1f into yegor256:master Feb 9, 2019
@rultor
Copy link
Collaborator

rultor commented Feb 9, 2019

@rultor merge

@llorllale Done! FYI, the full log is here (took me 13min)

@0crat
Copy link
Collaborator

0crat commented Feb 9, 2019

Code review was too long (10 days), architects (@llorllale) were penalized, see §55

@0crat 0crat removed the scope label Feb 9, 2019
@0crat
Copy link
Collaborator

0crat commented Feb 9, 2019

The job #1049 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Feb 9, 2019

Order was finished: +15 point(s) just awarded to @fabriciofx/z

@0crat
Copy link
Collaborator

0crat commented Feb 9, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale/z

@longstone longstone mentioned this pull request Apr 6, 2019
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.

6 participants