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 Makefile with sync designation #55

Merged
merged 9 commits into from
Feb 15, 2024
Merged

Add Makefile with sync designation #55

merged 9 commits into from
Feb 15, 2024

Conversation

coatless
Copy link
Collaborator

@coatless coatless commented Feb 8, 2024

Close #50 by implementing a make sync/make.

@coatless
Copy link
Collaborator Author

coatless commented Feb 9, 2024

Thinking about this more, I'll probably go back to the more "verbose" version in eb9cec7 instead of the symbolic rules to simplify the version injection.

@gadenbuie
Copy link
Owner

I think that's a good idea, it was also my first impression looking at the PR, but I hadn't had time to write back yet. I think it'll be a bit more approachable and we may need slightly different processes for version updating anyway.

@coatless
Copy link
Collaborator Author

coatless commented Feb 9, 2024

Okidokie, we have the version information being set in: lib/version.txt.

This information is then written into an appropriate config.{lua,R,py} file. From there, the version string is used in the relevant HTML dependency attachment.

Did you want the version information to be written into the asset files via a comment? (/* CSS countdownEmbedded: x.y.z */, // JS countDownEmbedded: x.y.z.) Or do you think the config file approach would be sufficient enough?

(P.S. The quarto CSS has local changes for RevealJS support that probably should go into the lib directory, c.f. #57 )

Copy link
Owner

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thanks for this! I updated the make file with a make help target that is used by default with make without arguments:

❯ make
debug              Print all variables for debugging
help               Show help messages for make targets
sync-all           Sync web assets to all subpackages
sync-python        Sync web assets to Python package
sync-quarto        Sync web assets to Quarto extension
sync-r             Sync web assets to R package

(This trick was picked up from https://github.com/krisnova/Makefile.)

I also re-arranged how we track source files and I renamed sync to sync-all so that it sorts correctly in the help menu. There's also a new make debug target to check the varaibles used in the Makefile.

@echo "Syncing web assets to the Python package..."
@test -d $(SYNC_DEST_PYTHON) || mkdir -p $(SYNC_DEST_PYTHON)
cp -r $(SRC_FILES) $(SYNC_DEST_PYTHON)
echo "countdown_version = '$(VERSION)'" > python/countdown/config.py
Copy link
Owner

@gadenbuie gadenbuie Feb 13, 2024

Choose a reason for hiding this comment

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

I like countdown_version a little better that countdown_embedded. There's a little room for ambiguity, but within each package, the unqualified version can be the package/extension version, e.g. packageVersion("countdown") in R or __version__ in Python.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okidokie, we could always do: asset_version or the more verbose countdown_asset_version if need be later on.

SYNC_DEST_R := r/inst/countdown
SYNC_DEST_QUARTO := quarto/_extensions/countdown/assets

sync-all: sync-python sync-r sync-quarto ## Sync web assets to all subpackages
Copy link
Owner

Choose a reason for hiding this comment

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

I renamed this because make help will sort targets by name and sync-all shows up first in the list while sync shows up after the other sync targets.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, the true reason why -all was needed :)

Copy link
Collaborator Author

@coatless coatless left a comment

Choose a reason for hiding this comment

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

The make help is a neat trick!

Thanks for adding in debug and simplifying the SRC_FILES merge.

Feel free to merge in whenever you have a moment.

SYNC_DEST_R := r/inst/countdown
SYNC_DEST_QUARTO := quarto/_extensions/countdown/assets

sync-all: sync-python sync-r sync-quarto ## Sync web assets to all subpackages
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, the true reason why -all was needed :)

@echo "Syncing web assets to the Python package..."
@test -d $(SYNC_DEST_PYTHON) || mkdir -p $(SYNC_DEST_PYTHON)
cp -r $(SRC_FILES) $(SYNC_DEST_PYTHON)
echo "countdown_version = '$(VERSION)'" > python/countdown/config.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okidokie, we could always do: asset_version or the more verbose countdown_asset_version if need be later on.

@gadenbuie gadenbuie merged commit c810f16 into main Feb 15, 2024
5 checks passed
@gadenbuie gadenbuie deleted the make-sync branch February 15, 2024 13:34
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.

Sharing lib assets
2 participants