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

Fix deep-merge for edn files #36

Merged
merged 3 commits into from
Feb 15, 2024
Merged

Conversation

brenopanzolini
Copy link
Contributor

@brenopanzolini brenopanzolini commented Feb 11, 2024

  • Please check if the PR fulfills these requirements
  • There is an open issue describing the problem that this pr intents to solve.
    • You have a descriptive commit message with a short title (first line).
  • Tests for the changes have been added (for bug fixes / features).
  • Docs have been added / updated (for bug fixes / features).
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This PR fixes a bug in the function that checks if a file path ends with .edn which is used to decide if the file will be merged.

The actual function has its arguments reversed:

#(.endsWith ".edn" (.getPath ^File %)) ;; Current
#(.endsWith (.getPath ^File %) ".edn") ;; New
  • What is the current behavior?
    The .edn files are not deep-merged during the build due to a bug in the function that checks if the file path ends with .edn.

  • What is the new behavior (if this is a feature change)?
    Not a new behavior, just fixing the logic to deep-merge .edn files.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No breaking changes.

  • Other information:
    Relates with: The deep-merge functionality for .edn files doesn't work #35

Comment on lines +23 to +24
(set? left)
(into left right)
Copy link
Contributor Author

@brenopanzolini brenopanzolini Feb 11, 2024

Choose a reason for hiding this comment

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

Hash sets are merged too.

(def left #{1 2})
(def right #{1 3})
(into left right)
;; => #{1 3 2}

Copy link
Contributor

@hlship hlship left a comment

Choose a reason for hiding this comment

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

This mistake I made, the actual bug, was that nothing exercised the base rules. It should be possible to have two copies of the same file on the classpath by introducing a second test resources route. Alternately, the rules could be unit tested seperate from the reading-and-merging logic.

@brenopanzolini brenopanzolini added the bug Something isn't working label Feb 13, 2024
@alan-ghelardi alan-ghelardi merged commit ad914e6 into master Feb 15, 2024
2 checks passed
@alan-ghelardi alan-ghelardi deleted the fix-merge-match-fn-logic branch February 15, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants