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

refactor: separate out logic for setting value #26

Closed
wants to merge 11 commits into from

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Dec 23, 2021

The starting point of this refactor was separating the logic for how flags and values are stored from the parsing to address #19. In the process, I identified a couple of likely bugs with = handling.

I took an interpretation of what should be stored in results for flags/values consistent with the new naming. (#13)

Possible Conventional Commit footers for commit comment (I have not used convention before):

feat!: values never an array unless "multiple"

feat!: flag option stored in `flags`, and *not* in `values` unless "multiple"

feat!: value option stored in `values`, and *not* in `flags`

fix!: `a=b` is treated as option `a` with value `b` whether or not marked as "withValue"

fix: `a=b=c` takes everything after first equals as value so value is `b=c`

@bcoe
Copy link
Collaborator

bcoe commented Dec 28, 2021

@shadowspawn all your assumptions seem sound in my opinion, and this refactor seems great 👍

@shadowspawn
Copy link
Collaborator Author

Thanks for the feedback, I wasn't sure if I was overreaching. 😅
I will update the tests.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Dec 28, 2021

Tests updated.

One of the tests did pick up a bug I introduced in the refactored code. 😄

@shadowspawn shadowspawn marked this pull request as ready for review December 28, 2021 05:43
Copy link
Collaborator

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid to me.

index.js Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator Author

This PR got mentioned in the recent Tooling Group meeting: https://docs.google.com/document/d/1Jt4XANUsahEv7JRUZqtSF1XVKWWPW1AWIHylBxLPbpU/edit

I'll wait for another review approval before merging, or the next meeting. Whichever comes first. 😄

index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
@shadowspawn shadowspawn changed the title Separate out logic for setting value feat: Separate out logic for setting value Jan 9, 2022
@shadowspawn shadowspawn changed the title feat: Separate out logic for setting value refactor: separate out logic for setting value Jan 9, 2022
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 18, 2022

This PR is in limbo with some of the spawned issues having changed the underlying code, and some of the changes at odds with original vision which has been freshly enumerated in #24 (comment)

Maybe a success on both counts to move the project forward. 😅

Waiting for some clarity on whether to rewrite same PR on new base, or whether to split into subsets. (The refactor will clash with everything else, so I don't want both that and others in flight at the same time.)

#24 (comment)

@shadowspawn
Copy link
Collaborator Author

I'll start again and try and land the refactor first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants