search

ocaml-semver / ocaml-api-watch

Libraries and tools to keep watch on you OCaml lib's API changes
ISC License
21 stars 15 forks source link

The diff comparison is not catching removed types #23

Closed shonfeder closed 8 months ago

shonfeder commented 8 months ago

As observed by @harshey1103 at https://github.com/NathanReb/ocaml-api-watch/pull/19#issuecomment-1985055868, and then demonstrated by @MungaSoftwiz in #16, the way we are using Includemod.signatures does not account for the removal of types.

@NathanReb has noted that we will probably end up moving away from Includemod.signatures, but in the meantime we should be able to account for this by also calling Includemod.signatuer with the arguments current and reference arguments flipped. This was also suggested by @harshey1103.

MungaSoftwiz commented 8 months ago

Can this issue be assigned to me once it's ready to be worked on please?

harshey1103 commented 8 months ago

Can I work on this issue since I have an idea how to do it and I have already implemented it in my local branch?

shonfeder commented 8 months ago

Hello! I think it makes sense for @harshey1103 to work on this issue, partially because they already have been touching this code directly in #19 and partially because they first called out this problem and proposed the interim solution.

@MungaSoftwiz, I will work on preparing an interesting issue for you. I should have something ready by tomorrow.

shonfeder commented 8 months ago

edited: removed a suggestion here because it was incorrect.

shonfeder commented 8 months ago

@harshey1103 this issue has a lot of overlap with #13, because both involve refactoring the core logic of the run function in bin/api_diff.ml. Depending which of these is completed first, we may need to resolve some merge conflicts. Just FYI. I am happy to help with this.

Please ignore my previous message!

harshey1103 commented 8 months ago

@shonfeder I have understood what we are trying to achieve in #13. I'd be happy to help @Siddhi-agg on #13, given my PR is merged before. We'll ask for your help if the need arises :)

MungaSoftwiz commented 8 months ago

Hello! I think it makes sense for @harshey1103 to work on this issue, partially because they already have been touching this code directly in #19 and partially because they first called out this problem and proposed the interim solution.

@MungaSoftwiz, I will work on preparing an interesting issue for you. I should have something ready by tomorrow.

Okay @shonfeder. Looking forward to it.

Siddhi-agg commented 8 months ago

@shonfeder I have understood this issue and since @harshey1103 has already created a PR for it, that has been reviewed and is almost done, I am continuously following its progress to know the changes I will require for #13.

@shonfeder I have understood what we are trying to achieve in #13. I'd be happy to help @Siddhi-agg on #13, given my PR is merged before. We'll ask for your help if the need arises :)

Sure! I would love to work with you, if any conflicts arise.

shonfeder commented 8 months ago

Hi, @MungaSoftwiz. Just a quick update that us mentors will need to talk on Monday to clarify the next steps for the project before opening more issues. Sorry for the delay.

MungaSoftwiz commented 8 months ago

Hi, @MungaSoftwiz. Just a quick update that us mentors will need to talk on Monday to clarify the next steps for the project before opening more issues. Sorry for the delay.

No problem @shonfeder. Thank you for the update.