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

rename modules to allow for further extension #18

Merged
merged 7 commits into from
Jan 30, 2022

Conversation

seanbreckenridge
Copy link
Owner

see CHANGELOG.md for thoughts

@seanbreckenridge
Copy link
Owner Author

seanbreckenridge commented Jan 30, 2022

@karlicoss curious if you had any thoughts here (I wrote up mine in the changelog)

I know similar things couldn't really be done on karlicoss/HPI since the dependent user base is so much higher (and often running in the background using promnesia and forgot about), so I thought I'd get ahead of it and remove possible module name conflicts from happening in the future

It is a breaking change, but I'd rather break it now in one commit than later when it becomes relevant for each module in particular, if that makes sense

If anyone is using my modules, its likely through my promnesia modules, so as long as they update both at the same time, there should be no issues

Related issues on main repo: extensions/overrides, reddit issue

@karlicoss
Copy link
Contributor

karlicoss commented Jan 30, 2022

Yep, agree, this looks good! Also new modules I'm writing are using a similar structure now.

So, say we switch promnesia to use from my.reddit.all import ... -- I guess the main failure modes we anticipate are

  1. Someone runs pip install -U promnesia (or git pull) -- it will update the promnesia reddit module, but will leave hpi intact? (because it's an optional dependency).
    Not much we can do about it (they'd have to update HPI as well), but the sad thing is that it's gonna result in some cryptic import errors. So we could add some checks to promnesia, e.g. against HPI version, or just checking whether my.reddit is a namespace package or regular package (i.e. old). Then could gracefully suggest the user to update HPI, without failing the whole indexing.

  2. Similarly, someone updates HPI, but forgets to update promnesia -- this is trickier, discussing in Zulip

@seanbreckenridge
Copy link
Owner Author

seanbreckenridge commented Jan 30, 2022

but the sad thing is that it's gonna result in some cryptic import errors. So we could add some checks to promnesia

yeah, this was one of my main concerns as well. Is true you can always handle it downstream, but it does start to add levels of complexity.

Is something I'll consider if I get around to writing an install/update command -- perhaps it could be used for both HPI/promnesia, and if it can just leverage something known like git submodule then this could always try to fetch the remote hash without merging, to let you know something has been updated remotely before causing some cryptic error. Still feels fairly complicated though

less likely these are overriden, but
might as well preempt them
@seanbreckenridge seanbreckenridge merged commit 8a19748 into master Jan 30, 2022
@seanbreckenridge seanbreckenridge deleted the rename-modules branch January 30, 2022 20:55
seanbreckenridge added a commit to seanbreckenridge/dotfiles that referenced this pull request Jan 31, 2022
seanbreckenridge added a commit that referenced this pull request Jan 31, 2022
karlicoss added a commit to karlicoss/HPI that referenced this pull request Feb 2, 2022
keeping it backwards compatible + conditional warning similar to #179

follow up for seanbreckenridge/HPI#18
for now without the __path__ hacking, will do it in bulk later

too lazy to run test_import_warnings.sh on CI for now, but figured I'd commit it for the reference anyway
karlicoss added a commit to karlicoss/HPI that referenced this pull request Feb 2, 2022
keeping it backwards compatible + conditional warning similar to #179

follow up for seanbreckenridge/HPI#18
for now without the __path__ hacking, will do it in bulk later

too lazy to run test_import_warnings.sh on CI for now, but figured I'd commit it for the reference anyway
karlicoss added a commit to karlicoss/HPI that referenced this pull request Feb 2, 2022
keeping it backwards compatible + conditional warning similar to #179

follow up for seanbreckenridge/HPI#18
for now without the __path__ hacking, will do it in bulk later

too lazy to run test_import_warnings.sh on CI for now, but figured I'd commit it for the reference anyway
karlicoss added a commit to karlicoss/HPI that referenced this pull request Feb 4, 2022
karlicoss added a commit to karlicoss/HPI that referenced this pull request Feb 4, 2022
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