search

NethermindEth / warp

Warp - Bringing Solidity to Starknet at warp speed. Warp is a Solidity to Cairo Compiler, this allows teams to write/migrate Solidity to Cairo for easy onboarding into the StarkNet ecosystem.
https://nethermind.io/warp/
Apache License 2.0
754 stars 70 forks source link

Remove an unused git submodule; fix error reporting in tests #1061

Closed temyurchenko closed 1 year ago

temyurchenko commented 1 year ago

Remove an unused git submodule; fix error reporting in tests

piwonskp commented 1 year ago

Please use a little more meaningful names and descriptions of PRs

temyurchenko commented 1 year ago

Please use a little more meaningful names and descriptions of PRs

Can you please suggest what the improvements would look like?

piwonskp commented 1 year ago

Please use a little more meaningful names and descriptions of PRs

Can you please suggest what the improvements would look like?

* I'm not sure what names refer to here.

* The PR consists of two rather tiny, unrelated commits, so I felt that there is unlikely to be a good unifying description of them.

* I tried to make commit messages meaningful, but I'm open to suggestions.

By name I meant the title of the PR. It could for example mention that the change is done to tests. You could also mention if it is just a code style fix or there is a change to the functionality. So basically a brief description of the reason for the change.

As for commit messages they're great. It's just that when reviewing PRs commit messages are often ommited since usually there are a lot of them

temyurchenko commented 1 year ago

Please use a little more meaningful names and descriptions of PRs

Can you please suggest what the improvements would look like?

* I'm not sure what names refer to here.

* The PR consists of two rather tiny, unrelated commits, so I felt that there is unlikely to be a good unifying description of them.

* I tried to make commit messages meaningful, but I'm open to suggestions.

By name I meant the title of the PR. It could for example mention that the change is done to tests. You could also mention if it is just a code style fix or there is a change to the functionality. So basically a brief description of the reason for the change.

As for commit messages they're great. It's just that when reviewing PRs commit messages are often ommited since usually there are a lot of them

I see. I added a short description for the PR. I am more inclined to considering commits as atomic, more-or-less contained changes, so I usually pay more attention to commit messages, furthermore given that they are preserved in git itself, while PR messages are just GitHub UI. (the team workflow prevails though)

temyurchenko commented 1 year ago

Oh, on the topic of workflows, the only option I have is to squash and merge. However, the commits are logically separate. Should I then turn this into 2 PRs? Seems a bit wasteful for such small changes, and kind of requires gather reviews anew, but I don't know if the alternative (allowing a merge commit on this change) is better. What do you think?

piwonskp commented 1 year ago

Please use a little more meaningful names and descriptions of PRs

Can you please suggest what the improvements would look like?

* I'm not sure what names refer to here.

* The PR consists of two rather tiny, unrelated commits, so I felt that there is unlikely to be a good unifying description of them.

* I tried to make commit messages meaningful, but I'm open to suggestions.

By name I meant the title of the PR. It could for example mention that the change is done to tests. You could also mention if it is just a code style fix or there is a change to the functionality. So basically a brief description of the reason for the change. As for commit messages they're great. It's just that when reviewing PRs commit messages are often ommited since usually there are a lot of them

I see. I added a short description for the PR. I am more inclined to considering commits as atomic, more-or-less contained changes, so I usually pay more attention to commit messages, furthermore given that they are preserved in git itself, while PR messages are just GitHub UI.

We use commit squashing so actually PR title is preserved in git itself :) It is more visible than individual commit messages

temyurchenko commented 1 year ago

Please use a little more meaningful names and descriptions of PRs

Can you please suggest what the improvements would look like?

* I'm not sure what names refer to here.

* The PR consists of two rather tiny, unrelated commits, so I felt that there is unlikely to be a good unifying description of them.

* I tried to make commit messages meaningful, but I'm open to suggestions.

By name I meant the title of the PR. It could for example mention that the change is done to tests. You could also mention if it is just a code style fix or there is a change to the functionality. So basically a brief description of the reason for the change. As for commit messages they're great. It's just that when reviewing PRs commit messages are often ommited since usually there are a lot of them

I see. I added a short description for the PR. I am more inclined to considering commits as atomic, more-or-less contained changes, so I usually pay more attention to commit messages, furthermore given that they are preserved in git itself, while PR messages are just GitHub UI.

We use commit squashing so actually PR title is preserved in git itself :) It is more visible than individual commit messages

I see, I changed the title as well. What do you think with regards to the second question?

piwonskp commented 1 year ago

Oh, on the topic of workflows, the only option I have is to squash and merge. However, the commits are logically separate. Should I then turn this into 2 PRs? Seems a bit wasteful for such small changes, and kind of requires gather reviews anew, but I don't know if the alternative (allowing a merge commit on this change) is better. What do you think?

I don't think it's a good idea to start mixing PR merging with squashing in the repository. I'd leave it as it is since it's already reviewed. Let's just squash and merge it

JorikSchellekens commented 1 year ago

Why are we removing the solidity submodule? We'll need it again when we're testing the semantics of the new version.

temyurchenko commented 1 year ago

Why are we removing the solidity submodule? We'll need it again when we're testing the semantics of the new version.

I made a project-wide search and couldn't find anything that used this path at this moment. Furthermore, on trying to initialize it, git complains about it with «fatal: No url found for submodule path 'tests/semantic/solidity' in .gitmodules» (or alternatively just doesn't pull anything). So, we have to fix that, but given that we don't use semantic tests at this moment, it might be better to just delete this one submodule and add it anew, fixed, when we get to them.