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

🚧 Rename ‘Assertions’ to ‘Expectations’ #748

Conversation

jGleitz
Copy link
Collaborator

@jGleitz jGleitz commented Dec 23, 2020

This is work in progress. This draft PR exists only for visibility of the progress and to detect conflicts early.


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

@jGleitz jGleitz requested a review from robstoll as a code owner December 23, 2020 23:14
@jGleitz jGleitz marked this pull request as draft December 23, 2020 23:14
@jGleitz jGleitz force-pushed the rename-to-expectation branch from 59e60bf to 4dd9b21 Compare December 23, 2020 23:16
@robstoll
Copy link
Owner

@jGleitz you need to change build.gradle of bc-test. Currently, we always create bc and bbc tests. It makes sense to split that and only keep bc for all old versions. We will introduce bbc again for 0.16.0. I am going to create a pull request deleting all deprecated stuff today probably in the late afternoon or evening where I will do this as well. So, you can also wait until I have merged my changes.

@@ -205,7 +205,7 @@ fun <T : CharSequence> Expect<T>.endsWith(expected: CharSequence): Expect<T> =
*
* @since 0.9.0
*
* @sample ch.tutteli.atrium.api.fluent.en_GB.samples.CharSequenceAssertionsSpec.endsWithChar
* @sample ch.tutteli.atrium.api.fluent.en_GB.samples.CharSequenceExpectationsSpec.endsWithChar
Copy link
Owner

@robstoll robstoll Dec 24, 2020

Choose a reason for hiding this comment

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

Looks like those were named wrong in the first place. Can you rename them to ...Samples instead of ...Spec (perfectly fine if you do this in a subsequent PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in this PR.

@robstoll
Copy link
Owner

@jGleitz I have merged my changes, so a rebase, accepting my changes in case of a conflict and re-do the renaming for those files should do it I guess

@robstoll
Copy link
Owner

I realised that the bc test is actually a half bbc test. I am pretty sure it will fail due to the renamings. You can ignore that. I am going to fix it

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 6, 2021

@robstoll This PR will break source compatibility all over the place. Might it make sense to remove all of the @Deprecated stuff that is not in the API first? A lot of it is announced to be removed with 1.0.0, however, if we rename everything anyway, is it worth keeping the baggage?

@jGleitz jGleitz force-pushed the rename-to-expectation branch from 4dd9b21 to f85a8b3 Compare January 6, 2021 18:45
@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 6, 2021

I am asking because I don’t assume that we want to duplicate every type that has ‘Assertion’ in it and mark it as deprecated. Since non-api artefacts are not considered stable, that would not be worth the effort, would it?

Until now, I have been careful not to rename anything that is deprecated. However, that makes the work more tedious. And it also feels silly: breaking compatibility with production code while trying to maintain it for deprecated types.

@robstoll
Copy link
Owner

robstoll commented Jan 6, 2021

A lot of it is announced to be removed with 1.0.0, however, if we rename everything anyway, is it worth keeping the baggage?

I don't like to remove stuff prior to an announcement in terms of a @Deprecated annotation. Since you are referring to such cases, it is probably OK to remove but I would prefer if we could discuss it first. I have removed quite a lot of stuff already (I assume you already rebased), so I am not sure of which things you talk about. Translations?

@robstoll
Copy link
Owner

robstoll commented Jan 6, 2021

If possible, it would be good if you could remove the stuff from this PR which you think could be controversial and only keep stuff which isn't (rename of samples, specs, files names). And then we can already merge this PR.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 6, 2021

I think this PR is uncontroversial in its current form. So I won’t push any further changes (docs and core are still missing), but only fix the bc tests. Then we can merge and I’ll open another PR for the next round.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 6, 2021

so I am not sure of which things you talk about.

ExpectationGroups, mainly. I’ll post details in the next PR.

@robstoll
Copy link
Owner

robstoll commented Jan 6, 2021

but only fix the bc tests

Please don't fix them (at least not if it is due to a binary backward compatibility issue), it would be a waste of time, I am currently re-working them.

ExpectationGroups, mainly. I’ll post details in the next PR.

Assertion and AssertionGroup should keep its name IMO. You write an expectation but an Assertion is created behind the scene, checked against your expectation, a report is created if it does not hold etc. I kind of like this distinction. I always disliked that I called the API functions "assertion function" which sounds like they create Assertions where in reality they did much more.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 6, 2021

I see what you are aiming at with the assertion / expectation distinction, although I don’t know whether it makes sense to my head. Currently, I would not yet know where exactly to draw the line. But since that would be part of another PR anyway, that’s fine, I guess.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 6, 2021

Please don't fix them (at least not if it is due to a binary backward compatibility issue)

The build fails because of the Source Backward Compatibility test. Should I attempt to fix it (I have not yet had a look at it) or not?

@robstoll
Copy link
Owner

robstoll commented Jan 6, 2021

The build fails because of the Source Backward Compatibility test. Should I attempt to fix it (I have not yet had a look at it) or not?

I would wait until I have the new bc-test setup, rebase and see if they still fail

@@ -1,12 +0,0 @@
package ch.tutteli.atrium.scala2.api.fluent.en_GB
Copy link
Owner

Choose a reason for hiding this comment

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

Should not have been deleted I guess

import ch.tutteli.atrium.creating.Expect
import ch.tutteli.atrium.logic.{IterableLikeKt, LogicKt}

class IterableLikeElementComparableExpectations[Repr, E <: Comparable[E]](override val expect: Expect[Repr])(
Copy link
Owner

Choose a reason for hiding this comment

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

same, don't delete

@@ -9,7 +9,7 @@ import kotlin.reflect.KClass
/**
* Collection of assertion functions and builders which are applicable to any type (sometimes `Any?` sometimes `Any`).
*/
interface AnyAssertions {
interface AnyExpectations {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should not do this renaming at the moment. If we don't rename Assertion (and I suggest we don't for the moment) then it makes more sense to call this AnyAssertions as all functions create an Assertion or a SubjectChangerBuilder.ExecutionStep (and in other files also FeatureExtractorBuilder.ExecutionStep)

Could you please revert the changes in the path /logic for now

Copy link
Owner

Choose a reason for hiding this comment

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

I am sure now that I will keep the name Assertion on the logic level. The main type on the logic level is also AssertionContainer instead of Expect.

@robstoll
Copy link
Owner

robstoll commented Jan 8, 2021

@jGleitz you can rebase, the new bc-tests are established

@robstoll robstoll changed the base branch from master to rename-assertion-to-expectation January 10, 2021 21:20
@robstoll
Copy link
Owner

As this PR will run into more and more conflicts, I am going to merge it into a different branch than master and cherry-pick the commits which I can use and then merge into master

@robstoll robstoll force-pushed the rename-assertion-to-expectation branch from 623158a to 3ba54bf Compare January 10, 2021 21:22
@robstoll robstoll marked this pull request as ready for review January 10, 2021 21:22
@robstoll robstoll force-pushed the rename-assertion-to-expectation branch from 3ba54bf to 79460ae Compare January 10, 2021 21:24
@robstoll robstoll merged commit 57a3e5d into robstoll:rename-assertion-to-expectation Jan 10, 2021
@robstoll
Copy link
Owner

robstoll commented Jan 10, 2021

I am came to realise that renaming the files in APIs is of no value for the moment. Since we are going to duplicate everything I prefer to keep the old files and introduce new files with the correct naming instead (same same for samples). This way we have a clear separation most likely without the need for bbc breaks - ah no... we will most likely have bbc breaks anyway since we have some inline methods in the API.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 10, 2021

I intentionally coordinated with you before working on this PR, so the work be for nought 😑

@robstoll
Copy link
Owner

I am very sorry about that, something I only realised when I wanted to merge your PR. I would have run into the same if I did the renaming... in this sense, thanks for doing it. I can still use some of the renaming and adjustments though:
#781

@jGleitz
Copy link
Collaborator Author

jGleitz commented Jan 10, 2021

It’s alright, I know these things happen. Sometimes you need to see it before having a definite opinion. I’d just like to have the little time I can spare for Atrium to have more impact 😄

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