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

deprecate midje support #174

Merged
merged 4 commits into from Mar 7, 2023
Merged

deprecate midje support #174

merged 4 commits into from Mar 7, 2023

Conversation

dchelimsky
Copy link
Contributor

@dchelimsky dchelimsky commented Sep 7, 2022

Problem

Within Nubank, we've standardized on clojure.test and deprecated the use of midje. Users who don't rely on midje support incur a transitive dependency on midje anyway.

Solution

  • annotate the matcher-combinators.midje namespace and its functions as deprecated
  • move midje dependency to :scope "provided" (midje has been moved to :extra-deps of :dev alias, see migrate to deps.edn #197)

Rationale

We don't intend to maintain midje support within this library any longer. We have no plan to remove the matcher-combinators.midje namespace outright, so those who depend on it will continue to work as before as long as you maintain an explicit dependency on it.

@dchelimsky dchelimsky changed the title move midje to scope provided deprecate midje support and move midje to scope provided Nov 10, 2022
src/clj/matcher_combinators/midje.clj Outdated Show resolved Hide resolved
src/clj/matcher_combinators/midje.clj Outdated Show resolved Hide resolved
@dchelimsky dchelimsky marked this pull request as ready for review November 10, 2022 23:20
@@ -0,0 +1,80 @@
{:hooks {:analyze-call {midje.sweet/tabular marick.midje/tabular}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@borkdude do you know if committing these configs here in combination with tagging midje as :scope "provided" in the matcher-combinators deps mean that the midje configs for clj-kondo would be still be picked up via clj-kondo --lint $(clojure -Spath) --dependencies --copy-configs ?

Like does --copy-configs copy all .clj-kondo configs from a lib even if it is from a different org namespace?

Choose a reason for hiding this comment

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

No, that won't cause configs to be copied. Only configs under resources/clj-kondo.exports are copied. The .clj-kondo directory isn't part of the distributed jar of this library.

CHANGELOG.md Outdated
@@ -3,6 +3,12 @@ All notable changes to this project will be documented in this file. This
change log follows the conventions of
[keepachangelog.com](http://keepachangelog.com/).

## [3.6.1]
- deprecate the matcher-combinators.midje ns and functions in it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fun to imagine this resulting in tens of thousands of new "deprecated" lint warnings in the test suites that Nubank still hasn't migrated away from Midje. Maybe this will help stoke the fire :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nobody cares about deprecation warnings except @acamargo and me.

@philomates
Copy link
Collaborator

maybe (too) dirty, but we could also move all the midje-related matcher-combinator stuff into midje itself, but still under the same matcher-combinator.midje namespace so that any midje users would see the same stuff/not require any code changes, but we could go clean from midje here

@scottbale
Copy link

scottbale commented Feb 14, 2023

Documenting here for clarity: Newer PR #197 deleted project.clj (which was where "scope provided" would have been specified). In its place, deps.edn confines midge dependency to the :extra-deps of :dev alias. (See also :midje-test alias.) I will rename this PR to make that more clear.

@scottbale scottbale changed the title deprecate midje support and move midje to scope provided deprecate midje support and move midje to :extra-deps of :dev alias Feb 14, 2023
@scottbale scottbale changed the title deprecate midje support and move midje to :extra-deps of :dev alias deprecate midje support Feb 14, 2023
CHANGELOG.md Outdated
Comment on lines 8 to 9
- move midje dependency to :scope "provided"
- this eliminates the transitive dep for those not using midje features

Choose a reason for hiding this comment

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

These two lines are no longer valid. #197 (since merged into this PR) removed project.clj and effectively already removed midje as a transitive dependency.

@dchelimsky
Copy link
Contributor Author

@philomates I updated the CHANGELOG and README. Please re-review. If it's good for you I'll go ahead and merge it.

@dchelimsky dchelimsky merged commit c5367d1 into master Mar 7, 2023
2 checks passed
@dchelimsky dchelimsky deleted the dc/midje-scope-provided branch March 7, 2023 18:00
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.

None yet

5 participants