search

AlloyTools / org.alloytools.alloy

Alloy is a language for describing structures and a tool for exploring them. It has been used in a wide range of applications from finding holes in security mechanisms to designing telephone switching networks. This repository contains the code for the tool.
Other
696 stars 124 forks source link

#127 - Add model counting function to Alloy #128

Closed jiayiyang1997 closed 2 years ago

jiayiyang1997 commented 3 years ago

Add model counting support for Alloy & extend the grammar of expect clauses to better meet users' requirements. Detailed info can be found here: https://github.com/AlloyTools/org.alloytools.alloy/issues/127.

pkriens commented 3 years ago

Could you update this to not have conflicts?

pkriens commented 3 years ago

I am happy we get PRs, so let me start with thanks! However, this is a massive PR and it is not directly clear to me what functionality you add? I think you provide semantics for the except part of the run clause? This actually has been discussed but it would be nice if you discussed this proposal on the alloytools.discourse.group list and try to get feedback on the idea & syntax. In general, we're very hesitant to add new keywords since they can break existing files so they need to go through some approval process with the Alloy board.

Also, when you commit a PR, minimize it. There are several people working on this code base and your changes will make it harder to merge. I actually see that there are many files actually unchanged, there is only a return removed or added. Also, the addition of the Intellij files is something I would not have a major concern with but should clearly be a distinct PR after a discussion. There are also binary jar files in the Git repository that should never be there.

Last, it is crucial to minimize the changes as I said. If you need to use a new constructor, like in the Command class, then create a new constructor. Do not update all uses of the old constructor, this unnecessarily changes many files that actually have not changed.

The problem with massive PRs is that they will take forever to review. The smaller the change, the easier it is to review & merge them.

pkriens commented 2 years ago

This PR has become outdated. I suggest you start a discussion on alloytools.discourse.group and get support for this idea.