dune icon indicating copy to clipboard operation
dune copied to clipboard

Published 20 hours ago • verified publisher icon ocaml

feat: add configurable typo detection for package dependency constraints

Open kemsguy7 opened this issue 6 months ago • 9 comments
trafficstars

kemsguy7 avatar May 05 '25 12:05 kemsguy7

@gridbugs , here's updates I made for issue #11629 .

kemsguy7 avatar May 05 '25 12:05 kemsguy7

@Leonidas-from-XIV @gridbugs , I've created updates for this pr here: https://github.com/ocaml/dune/pull/11766 . Please kindly review.

kemsguy7 avatar May 07 '25 04:05 kemsguy7

@kemsguy7 Why is that a separate PR? It would be better to push your improvements in this PR as then we can refer to the comments that were made in previous reviews.

Leonidas-from-XIV avatar May 07 '25 08:05 Leonidas-from-XIV

@Leonidas-from-XIV @gridbugs , please kindly review, I've applied updates ass suggested in the comments above

kemsguy7 avatar May 19 '25 06:05 kemsguy7

@kemsguy7 There are still review comments that I wrote that are unadressed, could you handle them please?

Also, it seems you need to solve a merge conflict in package_dependency.ml.

Leonidas-from-XIV avatar May 19 '25 07:05 Leonidas-from-XIV

@kemsguy7 There are still review comments that I wrote that are unadressed, could you handle them please?

Also, it seems you need to solve a merge conflict in package_dependency.ml.

Sure, please do you mind pointing out the parts that have been unaddressed If you have the time? I did a lot of rebasing, patching e.t.c to fix my PR as it got messy at some point so I might have reverted some of the files.

I'll take a second look though and also fix the merge conflict

kemsguy7 avatar May 19 '25 10:05 kemsguy7

Sure, please do you mind pointing out the parts that have been unaddressed If you have the time?

Sure. I checked the current state of the PR and these are still unaddressed:

  1. This one
  2. This one
  3. This one
  4. This one
  5. And this one

Leonidas-from-XIV avatar May 19 '25 10:05 Leonidas-from-XIV

@kemsguy7 the branch this PR is coming from does not include df45715469776a069fda5a4a39daabbb5675a0fe, since it's based on a version of origin/main from before your previous PR was merged. This is why it looks like the changes from your previous PR are duplicated in this PR which is making it hard to review.

Running git pull origin main --rebase (and dealing with any conflicts) will update your branch so that all your recent changes are applied to the current tip of origin/main.

gridbugs avatar May 20 '25 01:05 gridbugs

@kemsguy7 the branch this PR is coming from does not include df45715, since it's based on a version of origin/main from before your previous PR was merged. This is why it looks like the changes from your previous PR are duplicated in this PR which is making it hard to review.

Running git pull origin main --rebase (and dealing with any conflicts) will update your branch so that all your recent changes are applied to the current tip of origin/main.

Thank you for this. @Leonidas-from-XIV , @gridbugs please kindly review, I've updated with origin main and also implemented changes as requested

kemsguy7 avatar May 20 '25 10:05 kemsguy7