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

feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) #49

Merged
merged 14 commits into from
Dec 2, 2024

Conversation

techfg
Copy link
Contributor

@techfg techfg commented Apr 22, 2024

IMPORTANT - This PR contains the fixes in #55 so should be merged after it.

IMPORTANT: Breaking Change

If I'm understanding the way github-changelog-generator works, adding a label named Breaking changes: to this PR and/or issue #47 should annotate the generated changelog with the information about the breaking change. Alternatively, it looks like adding a label breaking to issue #47 would do the same thing so either way "should" work.

  1. Eliminates contentPath option
  2. Adds srcDir option
  3. Eliminates contentPathMode
  4. Adds collectionBase and collections options to align the application of collection base on a per collection basis rather than unilaterally to all collections

As detailed in #47, PR #2 had some shortcomings that PR #18 did not fully address. This PR addresses the remaining issues and creates the options needed to support future flexibility (e.g., #24 & #27).

FYI - I couldn't quite get the docs to emit inline the way I wanted so still working through that part. I did add a @see section and generated a separate interface doc for the new CollectionConfig options so the docs are covered, just not the way that I would ideally want them to generate. Might be a limitation with typedoc & zod or (hopefully) just operator error. I'll keep at it and submit separate PR if you agree with this one and it gets merged beforehand.

Resolves #47

@vernak2539
Copy link
Owner

I really like this idea, as it does open a lot of possibilities. I'm going to have to take a moment to load all the context back into my mind, so it may take me a few days! (i'm looking though!)

@techfg
Copy link
Contributor Author

techfg commented Apr 29, 2024

Sounds good! PR #50 which addresses Option 1 from #24 is based on this PR so this one should come first. This one also opens up the idea proposed in 53 which would build on both 50 & 24.

Ahead of this one though, would recommend you review 55, 36, 48, 51, 56 & 58 in that order first. They are all pretty small and built directly on top of main and should simplify merging for this one, 50 and the potentially 52 (review this one last and see all my comments in the PR regarding my current thoughts on it).

@techfg
Copy link
Contributor Author

techfg commented Nov 24, 2024

Hi @vernak2539 -

I merged latest from main into this PR to resolve the conflicts and hopefully makes things a bit easier on you assuming you move forward with merging this PR.

Note that the merge was not trivial due to the changes in this PR and changes in the other PRs that were merged back in May as well as recently. I believe I merged properly but given the nature of all the changes across the PRs, there is a chance that I missed something and/or merged incorrectly. All the tests pass and things should be good but just mentioning this in case any issues crop up (assuming this PR is merged) so we can circle back to this merge as the likely culprit.

@vernak2539 vernak2539 force-pushed the fix/remove-contentpath-option branch from 356df05 to 2cacf38 Compare December 2, 2024 14:55
@vernak2539 vernak2539 added the breaking Indicates a breaking change, which the changelog generator should pick up appropriately label Dec 2, 2024
@vernak2539
Copy link
Owner

Looks good to me! I've pushed a few changes to the PR (mostly to revert docs changes, which are generated automagically now after merges to main).

I've tested these changes by linking it to my blog, and everything seems great!

I'll merge #50 soon, then we can get a release out.

@vernak2539 vernak2539 changed the title fix: remove contentPath, add srcDir & align collection base on a per collection basis feat: Add srcDir & align collection base on a per collection basis (remove contentPath + contentPathMode) Dec 2, 2024
@vernak2539 vernak2539 merged commit b456988 into vernak2539:main Dec 2, 2024
6 checks passed
@techfg
Copy link
Contributor Author

techfg commented Dec 2, 2024

@vernak2539 -

Awesome, great to see this merged, thank you and glad to hear that it worked seamlessly with your blog!

Regarding docs, noted on the auto generate, I'll avoid generating on future PRs :) Also, FYI that I'll be submitting a PR to fix some typing and docs issues once #50 is merged since, as discussed on previous PRs, there are some shortcomings on both :)

@vernak2539 vernak2539 mentioned this pull request Dec 5, 2024
@techfg techfg mentioned this pull request Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Indicates a breaking change, which the changelog generator should pick up appropriately
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: contentPath is required in order to correctly transform when collectionPathMode is root
2 participants