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.

Sign up for GitHub

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

Jump to bottom

#630 add Path.isRelative method #632

Merged
merged 3 commits into from
Oct 16, 2020
Merged

#630 add Path.isRelative method #632

merged 3 commits into from
Oct 16, 2020

Conversation

jakubriegel
Copy link
Collaborator

Add Path.isRelative() for assertions and both APIs. From issue #630

Changes may be helpful for to #629

I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

@jakubriegel jakubriegel mentioned this pull request Oct 15, 2020
Closed
6 tasks
@jGleitz
Copy link
Collaborator

jGleitz commented Oct 15, 2020

Can you also add the German translations? They are ABSOLUTE_PATH("ein absoluter Pfad") and RELATIVE_PATH("ein relativer Pfad")

@jakubriegel
Copy link
Collaborator Author

Can you also add the German translations? They are ABSOLUTE_PATH("ein absoluter Pfad") and RELATIVE_PATH("ein relativer Pfad")

Ja wohl

@jakubriegel jakubriegel force-pushed the #630_path_is_relative branch 2 times, most recently from 09c7da5 to af317a5 Compare October 15, 2020 12:47
robstoll
robstoll previously approved these changes Oct 15, 2020
View reviewed changes
@robstoll robstoll linked an issue Oct 15, 2020 that may be closed by this pull request
add Path.isRelative #630
Closed
6 tasks
@robstoll
Copy link
Owner

@jakubriegel the PR looks good but the CI fails 😉
https://github.com/robstoll/atrium/pull/632/checks?check_run_id=1259105418#step:4:392

@jakubriegel
Copy link
Collaborator Author

@jakubriegel the PR looks good but the CI fails 😉
https://github.com/robstoll/atrium/pull/632/checks?check_run_id=1259105418#step:4:392

@robstoll trying to fix it with .normalize(). Unfortunately I can't reproduce that on Mac

@robstoll
Copy link
Owner

@jGleitz do you have time to take a look why it fails on linux?

@robstoll
Copy link
Owner

@jakubriegel btw. GitHub actions is Windows

robstoll
robstoll reviewed Oct 15, 2020
View reviewed changes
logic/atrium-logic-jvm/src/main/kotlin/ch/tutteli/atrium/logic/impl/DefaultPathAssertions.kt Outdated
@@ -147,6 +150,17 @@ class DefaultPathAssertions : PathAssertions {
.build()
}

private inline fun <T : Path> pathTypeAssertion(
Copy link
Owner

Choose a reason for hiding this comment

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

this is basically the same as fileTypeAssertion - can please you introduce a ioResultAssertion function which takes a typeTest: (IOResult<BasicFileAttribute>) -> Boolean can call it from file... and pathTypeAssertion

@jakubriegel jakubriegel force-pushed the #630_path_is_relative branch 2 times, most recently from 579a0e7 to 2e228fd Compare October 15, 2020 14:54
jGleitz
jGleitz reviewed Oct 15, 2020
View reviewed changes
.../atrium-logic-jvm/src/main/kotlin/ch/tutteli/atrium/logic/creating/filesystem/hints/hints.kt Outdated
provider: SubjectProvider<Path>
): Descriptive.DescriptionOption<DescriptiveAssertionWithFailureHint.FinalStep> =
withFailureHintBasedOnDefinedSubject(provider) { path ->
explainForResolvedLink(path) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think explainForResolvedLink is correct here. The assertion does not resolve symlinks (right?), so there should also be no explanation about the resolved target. The assertion should only be about the path itself, regardless of the file system content.

Copy link
Collaborator Author

@jakubriegel jakubriegel Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Simplified to

fun Descriptive.DescriptionOption<Descriptive.FinalStep>.withPathAttributesFailureHint(
    provider: SubjectProvider<Path>
): Descriptive.DescriptionOption<DescriptiveAssertionWithFailureHint.FinalStep> =
    withFailureHintBasedOnDefinedSubject(provider) { path ->
       describeWas(path.type)
            .let {
                assertionBuilder.explanatoryGroup.withDefaultType
                    .withAssertions(listOf(it))
                    .build()
            }
    }

jGleitz
jGleitz requested changes Oct 16, 2020
View reviewed changes
Copy link
Collaborator

@jGleitz jGleitz left a comment

Choose a reason for hiding this comment

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

After all the confusion, I think this PR is almost ready. I only found nit picks.

@jakubriegel you might want to consider configuring your IntelliJ to clean up imports before you commit:

optimize imports

...s/atrium-specs-jvm/src/main/kotlin/ch/tutteli/atrium/specs/integration/PathAssertionsSpec.kt Outdated Show resolved Hide resolved
...en_GB-jdk8-jvm/src/test/kotlin/ch/tutteli/atrium/api/fluent/en_GB/jdk8/PathAssertionsSpec.kt Outdated Show resolved Hide resolved
.../atrium-logic-jvm/src/main/kotlin/ch/tutteli/atrium/logic/creating/filesystem/hints/hints.kt Outdated
import ch.tutteli.atrium.logic.creating.transformers.impl.ThrowableThrownFailureHandler
import ch.tutteli.atrium.logic.creating.filesystem.Failure
import ch.tutteli.atrium.logic.creating.filesystem.IoResult
import ch.tutteli.atrium.logic.creating.filesystem.Success
import ch.tutteli.atrium.reporting.translating.Translatable
import ch.tutteli.atrium.translations.DescriptionBasic
import ch.tutteli.atrium.translations.DescriptionPathAssertion
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the added imports, they should now be obsolete, right?

...s/atrium-specs-jvm/src/main/kotlin/ch/tutteli/atrium/specs/integration/PathAssertionsSpec.kt Outdated
@@ -19,6 +19,7 @@ import org.spekframework.spek2.dsl.Skip.No
import org.spekframework.spek2.dsl.Skip.Yes
import org.spekframework.spek2.dsl.TestBody
import org.spekframework.spek2.style.specification.Suite
import java.io.File
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an unused import, isn’t it? If yes, please remove it!

@jakubriegel
Copy link
Collaborator Author

@jGleitz I will try with import optimization, but I've turned it off, cause I didn't want to mess with existing conventions

@jakubriegel jakubriegel force-pushed the #630_path_is_relative branch 2 times, most recently from e000f87 to 99831bc Compare October 16, 2020 14:18
@codecov
Copy link

codecov bot commented Oct 16, 2020 •
edited
Loading

Codecov Report

Merging #632 into master will decrease coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #632      +/-   ##
============================================
- Coverage     87.34%   87.10%   -0.24%     
  Complexity       16       16              
============================================
  Files           903      903              
  Lines          8116     8121       +5     
  Branches        374      374              
============================================
- Hits           7089     7074      -15     
- Misses          949      968      +19     
- Partials         78       79       +1
Flag Coverage Δ Complexity Δ
#bbc 72.75% <0.00%> (-0.05%) 0.00 <0.00> (ø)
#bc 57.22% <0.00%> (-0.04%) 0.00 <0.00> (ø)
#current 58.02% <100.00%> (-0.28%) 120.00 <0.00> (ø)
#current_windows ? ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
.../tutteli/atrium/api/fluent/en_GB/pathAssertions.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...tlin/ch/tutteli/atrium/api/infix/en_GB/keywords.kt 94.73% <100.00%> (+0.29%) 0.00 <0.00> (ø)
...h/tutteli/atrium/api/infix/en_GB/pathAssertions.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...c/generated/kotlin/ch/tutteli/atrium/logic/path.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...tutteli/atrium/logic/impl/DefaultPathAssertions.kt 86.44% <100.00%> (+0.23%) 0.00 <0.00> (ø)
...li/atrium/logic/creating/filesystem/hints/hints.kt 81.04% <0.00%> (-9.48%) 0.00% <0.00%> (ø%)

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 1dbef19...8103e7e. Read the comment docs.

@robstoll robstoll merged commit b5b945d into robstoll:master Oct 16, 2020
@robstoll
Copy link
Owner

@jakubriegel thanks and congrats to your first contribution to Atrium 🎉
There are more issues to tackle 😉

@jakubriegel
Copy link
Collaborator Author

@jakubriegel thanks and congrats to your first contribution to Atrium 🎉
There are more issues to tackle 😉

Thanks @robstoll and @jGleitz for providing me with opportunity! It was my first hactoberfest contribution and I hope not last 😃

@jakubriegel jakubriegel deleted the #630_path_is_relative branch October 17, 2020 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jGleitz jGleitz jGleitz approved these changes

@robstoll robstoll Awaiting requested review from robstoll

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add Path.isRelative
4 participants
@jakubriegel @jGleitz @robstoll @frikit