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

return errors where appropriate #21

Closed
wants to merge 27 commits into from
Closed

return errors where appropriate #21

wants to merge 27 commits into from

Conversation

perbu
Copy link
Contributor

@perbu perbu commented Nov 15, 2023

Currently the package returns a boolean indicating success.

I've also done some minor tweaks for readability and adhering to conventions.

… a bool indicating success. Some minor tweaks for readability and adhering to conventions.
@dmke dmke changed the base branch from master to v2 November 15, 2023 09:35
@dmke
Copy link
Member

dmke commented Nov 15, 2023

I've changed base from master to v2. I try to look into this further later (maybe tomorrow).

@perbu perbu closed this Nov 15, 2023
@perbu perbu reopened this Nov 15, 2023
@perbu
Copy link
Contributor Author

perbu commented Nov 15, 2023

(sorry, I didn't see you changed the PR, so I was opening a new one when I saw what you did)

@dmke
Copy link
Member

dmke commented Nov 15, 2023

Don't worry, the Github UI can be confusing at times...

(Looks like I need to update the CI tooling. The test matrix still targets Go 1.13 👀)

items.go Outdated Show resolved Hide resolved
@perbu
Copy link
Contributor Author

perbu commented Nov 15, 2023

oh. there are errstr all over the place now. I'll fix.

Copy link
Member

@dmke dmke left a comment

Choose a reason for hiding this comment

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

Looks good so far. I'll merge this soon, after I've reconfigured the CI pipeline.

lexer.go Outdated Show resolved Hide resolved
test_test.go Outdated Show resolved Hide resolved
@perbu
Copy link
Contributor Author

perbu commented Nov 17, 2023

Thanks for looking at this. Sorry for the errstr stuff.

The question is what we're suppose to do with the deprecated stuff. I can have a stab a clearing it out once this PR is merged, or perhaps you wanna do it? Let me know and I can plan accordingly.

@dmke
Copy link
Member

dmke commented Nov 17, 2023

Sorry for the errstr stuff.

Don't worry, things happen.

The question is what we're suppose to do with the deprecated stuff. I can have a stab a clearing it out once this PR is merged, or perhaps you wanna do it? Let me know and I can plan accordingly.

Feel free to clear them out, while you're at it :)

I've pushed a few commits to update dependencies and the CI workflows, so you may want to rebase your PR on top of them.

@perbu
Copy link
Contributor Author

perbu commented Nov 19, 2023

Removed the deprecated calls as requested. Gone over the existing todos and resolved as many as I could. There is one left, which is wrt to a potential error which might show up during handling anonymous sections, something I know know nothing about.

I've been unable to figure out how to get switch my fork to use the v2 branch directly, so I haven't been able to rebase. Hope it doesn't cause much problems.

I'm quite happy with the state of it as it is now. Lemme know.

@dmke
Copy link
Member

dmke commented Nov 19, 2023

I've been unable to figure out how to get switch my fork to use the v2 branch directly

Assuming git remote -v displays something like

origin	[email protected]:perbu/go-uci (fetch)
origin	[email protected]:perbu/go-uci (push)
upstream	[email protected]:digineo/go-uci.git (fetch)
upstream	[email protected]:digineo/go-uci.git (push)

i.e. upstream points to this repo, and origin to yours, to rebase you'll need to:

  1. Fetch my v2 branch

    $ git fetch upstream 
  2. Move your commits onto that branch:

    $ git rebase upstream/master

    Note: This may or may not) create a merge conflict you'd need to resolve before continuing (I've fixed some linter issues, which you might have as well - if we've applied the same solution, git won't complain). Should things go south, you can abort the rebase attempt with git rebase --abort, even after solving one or more conflicts.

  3. Push your changes (forcefully):

    $ git push --force-with-lease origin master

Would you try that? If not, or if problems arise, let me know, I should be able to do this on my end.

… a bool indicating success. Some minor tweaks for readability and adhering to conventions.
@perbu
Copy link
Contributor Author

perbu commented Nov 20, 2023

I think I managed to pull it off.

@codecov-commenter
Copy link

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (37c7b10) 89.85% compared to head (570bdc9) 87.97%.
Report is 4 commits behind head on v2.

Files Patch % Lines
uci.go 77.27% 9 Missing and 1 partial ⚠️
errors.go 50.00% 2 Missing ⚠️
parser.go 50.00% 1 Missing ⚠️
types.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v2      #21      +/-   ##
==========================================
- Coverage   89.85%   87.97%   -1.88%     
==========================================
  Files           7        7              
  Lines         680      682       +2     
==========================================
- Hits          611      600      -11     
- Misses         60       73      +13     
  Partials        9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmke
Copy link
Member

dmke commented Nov 20, 2023

Looks good. I'll merge this when I'm back home.

@dmke
Copy link
Member

dmke commented Nov 20, 2023

I've squashed your commits into 60c1481, tagged as v2.0-rc.1 and cleaned some minor things up. The PR somehow contained a merge commit with master, which led to the commits being duplicated... Not sure how this could happen :)

There still was a (*tree).Set method lying around, but Set was already removed from the Tree interface. I'm now considering renaming SetType to Set, since one argument already contains the Type keyword:

uci.SetType("pkg", "sect", uci.TypeOption, "val")
// vs.
uci.Set("pkg", "sect", uci.TypeOption, "val")

@dmke dmke closed this Nov 20, 2023
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