Skip to content

Add git submodules for specs - #209

Merged
mielvds merged 4 commits into
masterfrom
181-add-submodules
Dec 4, 2025
Merged

Add git submodules for specs#209
mielvds merged 4 commits into
masterfrom
181-add-submodules

Conversation

@anatoly-scherbakov

@anatoly-scherbakov anatoly-scherbakov commented Nov 9, 2025

Copy link
Copy Markdown
Collaborator

I think my previous PR was trying to change too much and I decided it would be easier to recreate the most important changes in a new one.

This PR:

  • Creates two submodules (as discussed, normalization which is now rdf-canon is out of scope for now),
  • Wires them into CI (thanks @davidlehn for introducing GitHub Actions!)

@anatoly-scherbakov anatoly-scherbakov self-assigned this Nov 9, 2025
@anatoly-scherbakov
anatoly-scherbakov changed the base branch from master to add-github-workflow November 9, 2025 13:06

@mielvds mielvds left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm in favor if this approach because the advantages outweigh the disadvantages. If this ever changes, it's not that much work to change it.

@anatoly-scherbakov as a sort of compromise: should we add a 'testing without submodules' paragraph to the README.md? Basically what you said in #182 (comment) about changing the path of the specs.

@anatoly-scherbakov

Copy link
Copy Markdown
Collaborator Author

@mielvds sure; this PR, however, doesn't change the behavior of the test system. It only affects existence of submodules and usage of them in CI.

We can update the docs in another PR. Making PRs smaller could speed up merging.

@mielvds

mielvds commented Nov 12, 2025

Copy link
Copy Markdown
Collaborator

It kind of changes the test system, because contributors now need to init submodules instead of cloning them (although they still could). I'm all for small PR's, but I also like them to include documentation about the changes they introduce. It's a balance.
Also based on @davidlehn's concern in the previous PR:

Adding this feature forces the maintainers to learn to use it. Help us (me) out with a few more snippets of docs perhaps in the README and/or CONTRIBUTING. Maybe even more script/makefile targets to run. Specifically, what do we (I) need to type to keep the test suites synced up all the time.

So makefile is covered by #191, but I would add the snippets here because it's not that much work (I can add this).

@anatoly-scherbakov

Copy link
Copy Markdown
Collaborator Author

need to init submodules instead of cloning them

After this PR, contributors are not yet required to do so, they might continue cloning specs separately as before. This PR prepares the repo for a submodule-aware workflow.

In a follow up PR, I will:

  • Update runtests.py to use submodules by default;
  • Update documentation,

if that makes sense. Let me know what you think.

@mielvds

mielvds commented Nov 12, 2025

Copy link
Copy Markdown
Collaborator

allright with me! if possible, i'd like to wait for @davidlehn and @BigBlueHat for the final go?

@anatoly-scherbakov

Copy link
Copy Markdown
Collaborator Author

Thanks! I would also prefer the upstream, add-github-workflow to be merged first. On its merge, this PR will be automatically redirected to master and could be thereafter merged.

Base automatically changed from add-github-workflow to master November 14, 2025 23:38
@mielvds

mielvds commented Dec 4, 2025

Copy link
Copy Markdown
Collaborator

@anatoly-scherbakov can you rebase so we can merge?

@anatoly-scherbakov

Copy link
Copy Markdown
Collaborator Author

@mielvds done!

@mielvds
mielvds merged commit 6dc3cb5 into master Dec 4, 2025
17 checks passed
@mielvds
mielvds deleted the 181-add-submodules branch December 4, 2025 19:08
@davidlehn

Copy link
Copy Markdown
Member

Why were the normalization tests removed? That eventually needs to change to the rdf-canon ones, but seems like they should remain for now to ensure no regressions from current behavior.

@mielvds mielvds mentioned this pull request Dec 5, 2025
@mielvds
mielvds restored the 181-add-submodules branch December 5, 2025 09:18
@mielvds

mielvds commented Dec 5, 2025

Copy link
Copy Markdown
Collaborator

@anatoly-scherbakov I agree with @davidlehn , sorry I glanced over this; I should not have merged this yet. I have brought back normalization to keep the changes incremental and added to the README.rst in #218

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