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

Fix dips, nimble file handling, and gitops #120

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

elcritch
Copy link
Contributor

@elcritch elcritch commented Mar 11, 2024

This PR started to work on #117 but uncovered a few other items which needed cleaned up or fixed.

Changes:

  • Combine multiple ways to search for Nimble files into one place
  • fixes some edge cases in handling finding Nimble files
  • fixes issue with writing default config file and handling the deps cli option
  • unit tests for various nimble file states (missing, ambiguous, etc)
  • unit tests for parsing PkgUrl's
  • merges adhoc git execs to use the gitops module, restoring logging etc
  • cleanup for setting up triple-name on conflicts

@elcritch elcritch marked this pull request as ready for review March 11, 2024 15:07
@elcritch elcritch marked this pull request as draft March 11, 2024 16:37
@elcritch elcritch marked this pull request as ready for review March 29, 2024 14:13
This PR started to work on nim-lang#117 but uncovered a few other items which needed cleaned up or fixed.

Changes:

Combine multiple ways to search for Nimble files into one place
fixes some edge cases in handling finding Nimble files
fixes issue with writing default config file and handling the deps cli option
osutils.nim provides shims for unit testing proc's using a subset of file operations
unit tests for various nimble file states
unit tests for parsing PkgUrl's
fixes some adhoc git execs to use the gitops module, restoring logging etc
@elcritch
Copy link
Contributor Author

@Araq fixed this PR. I started tracking down another set of issues and got a broken commit mixed in here.

src/atlas.nim Outdated Show resolved Hide resolved
src/nimbleparser.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented May 13, 2024

Option[string] is actually worse than string if s.len == 0 is already an errornous state. You added yet another error state on top of the one we already have. For Nimble file detection it's something like:

type
  NimbleFile = object
     results: int
     file: string # only valid if results == 1

src/nimbleparser.nim Outdated Show resolved Hide resolved
src/nimbleparser.nim Outdated Show resolved Hide resolved
Don't error on ambiguous nimble files
src/atlas.nim Outdated Show resolved Hide resolved
src/atlas.nim Outdated Show resolved Hide resolved
src/atlas.nim Outdated Show resolved Hide resolved
Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
@elcritch
Copy link
Contributor Author

elcritch commented May 13, 2024

Option[string] is actually worse than string if s.len == 0 is already an errornous state. You added yet another error state on top of the one we already have.

It does introducing another possible error state, but that can be handled by ensuring the code that finds the nimble files doesn't return some(""). It's not, ideal, granted to be able to make an invalid state but it's localized to that single proc so should be easy to vet.

Mainly I found it useful in documenting elsewhere in the codebase that the Nimble file might not be found yet and that using .get will result in an easier to trace defect vs an empty string which might go unchecked.

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.

None yet

2 participants