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

🚧 Adopt the ‘to+infinitive’ Convention #714

Closed
wants to merge 1 commit into from
Closed

Conversation

jGleitz
Copy link
Collaborator

@jGleitz jGleitz commented Dec 13, 2020

Let’s get the ball rolling!

This PR will introduce the ‘to+infinitive’ convention for atrium.
It will not be merged before the convention has been applied to all of the fluent API.
It exists to gather feedback and find merge conflicts during the transition.

@jGleitz jGleitz marked this pull request as draft December 13, 2020 16:48
@jGleitz jGleitz changed the title Adopt the ‘to+infinitive’ Convention 🚧 Adopt the ‘to+infinitive’ Convention Dec 13, 2020
@jGleitz jGleitz changed the title 🚧 Adopt the ‘to+infinitive’ Convention 🚧 Adopt the ‘to+infinitive’ Convention Dec 13, 2020
@jGleitz
Copy link
Collaborator Author

jGleitz commented Dec 13, 2020

You’ll notice that I made cosmetical adaptions to the docs of the new functions. I think they are improvements and am happy to discuss them!

@jGleitz jGleitz force-pushed the to+infinitive branch 2 times, most recently from 922c5c0 to 76b4531 Compare December 13, 2020 17:03
@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #714 (e26ed64) into master (e9da7cf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #714   +/-   ##
=========================================
  Coverage     87.59%   87.59%           
  Complexity       16       16           
=========================================
  Files           904      904           
  Lines          8157     8159    +2     
  Branches        378      378           
=========================================
+ Hits           7145     7147    +2     
  Misses          935      935           
  Partials         77       77           
Flag Coverage Δ Complexity Δ
bbc 80.16% <100.00%> (+<0.01%) 0.00 <0.00> (ø)
bc 58.44% <0.00%> (-0.02%) 0.00 <0.00> (ø)
current 58.59% <75.00%> (-0.02%) 0.00 <0.00> (ø)
current_windows 49.29% <75.00%> (-0.02%) 0.00 <0.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
.../tutteli/atrium/logic/impl/DefaultAnyAssertions.kt 100.00% <ø> (ø) 0.00 <0.00> (ø)
...h/tutteli/atrium/api/fluent/en_GB/anyAssertions.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...ch/tutteli/atrium/api/infix/en_GB/anyAssertions.kt 66.66% <100.00%> (ø) 0.00 <0.00> (ø)
...rc/generated/kotlin/ch/tutteli/atrium/logic/any.kt 100.00% <100.00%> (ø) 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 e9da7cf...e26ed64. Read the comment docs.

@@ -30,22 +30,56 @@ fun <T> Expect<T>.notToBe(expected: T): Expect<T> = _logicAppend { notToBe(expec
/**
* Expects that the subject of the assertion is the same instance as [expected].
*
* Deprecated as atrium moves to a consistent ‘to + infinitive’ naming convention. Use [toBeTheSameAs] instead.
Copy link
Owner

Choose a reason for hiding this comment

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

Detail but more aligned with the readme:

Suggested change
* Deprecated as atrium moves to a consistent ‘to + infinitive’ naming convention. Use [toBeTheSameAs] instead.
* Deprecated as Atrium moves to a consistent ‘to + infinitive’ naming convention. Use [toBeTheSameAs] instead.

"Replaced by toBeTheSameAs. Will be removed with version 1.0.0",
ReplaceWith("this.toBeTheSameAs<T>(expected)")
) // TODO remove with 1.0.0
fun <T> Expect<T>.isSameAs(expected: T): Expect<T> = toBeTheSameAs(expected)
Copy link
Owner

Choose a reason for hiding this comment

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

@jGleitz IMO we should provide once again a script in order that users have a smooth migration experience. Do you see the draft release here? https://github.com/robstoll/atrium/releases/tag/untagged-c4eefbbdd8d22b72bc20

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you see the draft release

No, I get a 404 for that link.

Writing a script feels like duplicating what the @ReplaceWith annotations do anyway. Isn’t there any IntelliJ functionality to say ‘perform all migrations’?

Copy link
Owner

Choose a reason for hiding this comment

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

Not that I know of, you were able to choose Replace for all for a particular ReplaceWith but last time I wanted to use it I couldn't find the option. Moreover, from my experience, the applications of the ReplaceWith by Intellij are many times buggy and don't work as they should. I was always happy that I had the scripts. I'll put a draft of the release note into the wiki so that you could provide the replacements as well
Up to you if you want to help out or not. It's also fine if I collect/write down the replacements at the end of 0.16.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. I’ll prioritize work on my open pull requests, but will help on the script if I find the time.

@robstoll
Copy link
Owner

closing this in favour of #866 (other parts have been already merged into master via other PRs)

@robstoll robstoll closed this Apr 13, 2021
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.

2 participants