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

Naming and its align #66

Merged
merged 6 commits into from
Oct 26, 2023
Merged

Naming and its align #66

merged 6 commits into from
Oct 26, 2023

Conversation

skaunov
Copy link
Collaborator

@skaunov skaunov commented Oct 20, 2023

See #64 for details. This additional PR created for merging conflict resolution.

[Discussion] results are incorporated. Naming groomed repository wide. \
Fun fact: I already was confused by that "hash2" naming, lol.

Note that `fn verify_signals` barely [benefit]
from renaming at all currently.

[Discussion]: 251fba6#commitcomment-130400727
[benefit]: #61
@skaunov skaunov self-assigned this Oct 20, 2023
@skaunov skaunov linked an issue Oct 20, 2023 that may be closed by this pull request
@skaunov skaunov requested a review from Divide-By-0 October 20, 2023 17:34
@skaunov skaunov mentioned this pull request Oct 20, 2023
Closed
@skaunov
Copy link
Collaborator Author

skaunov commented Oct 20, 2023

@Divide-By-0 I suggest merging commit without squashing -- could help for further debugging a bit if anything.
Check and merge/close this one, pls.

@Divide-By-0
Copy link
Member

There shouldn't be both package lock and yarn lock :)

@skaunov skaunov mentioned this pull request Oct 22, 2023
Merged
@Divide-By-0
Copy link
Member

Hey I'd love to merge but there are some conflicts, do you mind resolving those first?

@skaunov
Copy link
Collaborator Author

skaunov commented Oct 25, 2023

Actually I did. Twice. X)) To absorb #48 . So there was "22_resolveConflict" branch (now deleted), then this one with trailing "_", and it had the nice green button to choose between squash & merge commit. Then some of the following bump commits reintroduced the conflict. =((

Long story short: the problem I see here is that these lock files have substantial size and it's better to minimize the back and forth with them (last time I saw was via deprecated npm cache clean force instead of current verify). Just because we have monorepo which already have a mix of things, and lock-files cycles unproductively grow the repo. =(

I'm not that good in Git to touch the repo with force, though I see it as a most appropriate solution here. Another ways I see are following.

  1. Revert bumping commits and merge PRs. But I believe it still will have that stuff in the revert commit. Or
  2. I can do again what I've done while merging Fix array signals #48: delete both conflicting files and regenerate <javascript/yarn.lock>. I felt it's ok to do that once and live happily with them further; but as regular practice I feel it's bad. =(

@Divide-By-0
Copy link
Member

Yikes, good feedback. I'm open to any structural changes you'd recommend!

For one we should just gitignore package lock, overwriting yarn lock seems alright, I agree it's not an ideal practice. I don't really have any ideas here; maybe we can edit our dependabot settings somehow, or cut these lock files entirely and make versioning in package json more explicit?? Idk, I'm OK with whatever you choose.

@skaunov
Copy link
Collaborator Author

skaunov commented Oct 26, 2023

Yeah, sounds like a good idea to .gitignore the <javascript/package-lock.json> as a "dirty fix" to prevent reintroducing the problem by accident! Will do it.

So I currently stick to the path where I revert commits starting from https://github.com/plume-sig/zk-nullifier-sig/compare/f4b53bc1835aaccd114f9ab070b3cac381fabf04..main, and then merge this PR. This course let isolate the bloat in that one reverse commit so that someone fluent in Git could remove dead weight with force.

Will tackle this now or then.

skaunov added a commit that referenced this pull request Oct 26, 2023
skaunov added a commit that referenced this pull request Oct 26, 2023
skaunov added a commit that referenced this pull request Oct 26, 2023
* Delete javascript/yarn.lock
* Delete javascript/package-lock.json
@skaunov skaunov merged commit f5dbd6a into main Oct 26, 2023
5 checks passed
@skaunov skaunov deleted the 22_resolveConflict_ branch July 3, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent naming across different implementations
2 participants