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

merged standard and augmented BNF parsing, removed ABNF feature #170

Closed
wants to merge 19 commits into from

Conversation

Carlyle-Foster
Copy link
Contributor

the lines added/deleted is very misleading because i move all the files out of the parsers folder and into a single parsers.rs file

@Carlyle-Foster
Copy link
Contributor Author

the amount of commits is also very misleading, i should probably learn how to use git rebase at this point

@coveralls
Copy link

Coverage Status

coverage: 97.02% (-0.09%) from 97.109%
when pulling 19af269 on Carlyle-Foster:main
into a8e52a3 on shnewto:main.

@Carlyle-Foster
Copy link
Contributor Author

hmm, the moving of files seems to have obscured the changes made to the parser, that's unfortunate but if u just skim the parser code it should be pretty clear what's going on, the core changes are very simple

@shnewto
Copy link
Owner

shnewto commented Feb 23, 2025

@Carlyle-Foster I appreciate all your work! I'm going to close this PR though and favor you opening another one though. There's no need to rush I think, so lets try and get the commit history and diff in the PR right. Also things like restructuring the project should be its own PR without other changes, with a really clear description to justify why you think its the right design decision. Thanks again for all your efforts, looking forward to the next round!

@shnewto shnewto closed this Feb 23, 2025
@Carlyle-Foster
Copy link
Contributor Author

@shnewto why does my fork show in github as 20 commits ahead? some of them are merge commits at which i synced with upstream, so why would any commit before them be considered "ahead"?

@shnewto
Copy link
Owner

shnewto commented Feb 23, 2025

it's a good question! sometimes force pushes will make history unintuitive, but also changes on top of and in the middle of merge commits can make history hard to reason about. i don't know for sure in this case but sometimes, I'll just create a fresh branch off upstream main, then copy my proposed changes to that new branch and make a single detailed commit for a cleaned up PR. you lose the history of the other branch, but for these PRs we'd be squashing on merge anyway 😄

@Carlyle-Foster
Copy link
Contributor Author

Carlyle-Foster commented Feb 24, 2025

@shnewto ur suggestion almost worked, but for some reason whenever i try to merge my main branch onto the new branch it adds the unsquashed commits b4 the squashed commits in the git history. to be clear i'm running
git branch new_main <old common commit>
git switch new_main
git merge upstream/main

(all good so far), but then it breaks on
git merge main

i've tried
git rebase main too but it had the same problem.

EDIT: nvm, git merge --squash does the trick!

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.

3 participants