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

add public function to generate docs and remove old files #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glours
Copy link

@glours glours commented Nov 2, 2023

Since we're using the alpha command in Compose, we need to remove sub commands which may either be remove or promote as official commands.

See docker/compose#11155 as example

@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (3895c94) 67.36% compared to head (7470ed4) 64.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
- Coverage   67.36%   64.45%   -2.91%     
==========================================
  Files           4        4              
  Lines         576      602      +26     
==========================================
  Hits          388      388              
- Misses        132      158      +26     
  Partials       56       56              
Files Coverage Δ
clidocstool.go 37.23% <0.00%> (-14.24%) ⬇️

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

@glours
Copy link
Author

glours commented Nov 2, 2023

@crazy-max @thaJeztah sorry to ping you but I can't add reviewers in this repo

@thaJeztah
Copy link
Member

👋 sorry, saw the PR yesterday, but didn't have time to comment.

I'm ever-so-slightly wondering what the best approach is for these cases, although not "strongly" attached (this repository is a tool/utility and not a polished end-product, so "whatever works, even if quirky, or opinionated" is probably "fine").

Effectively, it's similar to, e.g., go mod vendor, which can result in updated files, new files, but also files being removed; we do that in a container in various repositories, and it works®.

I guess the difference would be that go mod vendor only acts on files and directories that it owns (i.e., nobody should make manual changes to the vendor/ directory). For docs, it's possible for both generated and manually maintained files to be in in the same directory; so I'm wondering if that's the issue you ran into (if not, then perhaps the same approach as our vendor steps would work, which effectively does an rsync between a "fresh vendor' and "actual vendor").

My concern is somewhat (but again: it's a quirky tool, so "whatever works") that we wouldn't be able to tell if the generator "OWNS" a file just by looking at its name; I could have a manually maintained docker_foo.md, and the tool wouldn't know "was that created by an older version of me?" or "was this a file someone else created?"

@thaJeztah
Copy link
Member

@crazy-max any good suggestions?

(I'm not blocking this PR though if this is the best solution)

@crazy-max
Copy link
Member

Would have been better with a variadic opt on GenAllTree instead of new public func but sounds good for now. Otherwise yeah if source and dest collide it's a bit tricky to figure out what should be removed.

@glours
Copy link
Author

glours commented Nov 7, 2023

@crazy-max I created a new public function to avoid mis-usage of the original GenAllTree 😅
I'm fine to pass an Option structure to GenAllTree for activating the delete process

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.

4 participants