Skip to content

Move make_identifier function to collect.py#1215

Merged
ryneeverett merged 3 commits into
GothenburgBitFactory:developfrom
Lotram:move-identifier-function
Jun 2, 2026
Merged

Move make_identifier function to collect.py#1215
ryneeverett merged 3 commits into
GothenburgBitFactory:developfrom
Lotram:move-identifier-function

Conversation

@Lotram
Copy link
Copy Markdown
Collaborator

@Lotram Lotram commented May 19, 2026

in aggregate_issues, we have both the Issue object and the deserialized data, so we can compute the identifier without relying on a fragile iteration on list of key_lists Also add some more precise typing

Comment thread bugwarrior/types.py Outdated
Comment thread bugwarrior/types.py Outdated
Comment thread bugwarrior/collect.py Outdated
@Lotram Lotram force-pushed the move-identifier-function branch from e8e23de to 3315a89 Compare May 28, 2026 11:46
@Lotram
Copy link
Copy Markdown
Collaborator Author

Lotram commented May 28, 2026

Turns out I had a types.py to avoid cyclic dependency issues. I changed two things:

  • get_service is now defined in services/__init__.py, as it makes more sense than to define it in collect.py
  • config/__init__.py now only import (and re-export) objects used in the public API. It used to import all files, creating cyclic dependencies much more "easily".

@Lotram Lotram force-pushed the move-identifier-function branch from 3315a89 to b2bb879 Compare May 28, 2026 11:55
in aggregate_issues, we have both the Issue object
and the deserialized data, so we can compute the identifier
without relying on a fragile iteration on list of key_lists
Also add some more precise typing
@Lotram Lotram force-pushed the move-identifier-function branch from b2bb879 to 56754e9 Compare May 29, 2026 07:53
@ryneeverett
Copy link
Copy Markdown
Collaborator

Turns out I had a types.py to avoid cyclic dependency issues. I changed two things:

* `get_service` is now defined in `services/__init__.py`, as it makes more sense than to define it in `collect.py`

* `config/__init__.py` now only import (and re-export) objects used in the public API. It used to import all files, creating cyclic dependencies much more "easily".

It isn't clear to me that these changes are desirable. Could they be dropped now that the types are in collect.py?

@Lotram
Copy link
Copy Markdown
Collaborator Author

Lotram commented May 29, 2026

I'd keep the change on get_service. IMO, there is really no good reason to have it in collect.py. For now, config, as a modules, depends on collect.py, but it should not. If not in services/__init__.py, I think it should be somewhere in config/ (maybe validation.py ?), so that config does not depend on anything.

If we make sure of that (config not depending on anything), then I can revert the change on config/__init__.py, since we're sure that importing all config files can't be an issue for other modules.

@ryneeverett
Copy link
Copy Markdown
Collaborator

Keeping get_service in services/__init__.py and reverting the config/__init__.py changes makes sense to me. (Now that I look at get_service from a dependency perspective I see what you mean.)

get_service function is now defined in config
@Lotram Lotram force-pushed the move-identifier-function branch from 56754e9 to d96f589 Compare June 1, 2026 07:59
@Lotram
Copy link
Copy Markdown
Collaborator Author

Lotram commented Jun 1, 2026

I moved get_service definition to avoid a dependency config --> services --> config.
Config now depends only on itself, no imports from other bugwarrior files.

And I removed changes on config/__init__.py

Comment thread bugwarrior/collect.py Outdated
Comment thread bugwarrior/collect.py Outdated
@ryneeverett ryneeverett merged commit bd3c036 into GothenburgBitFactory:develop Jun 2, 2026
8 checks passed
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.

2 participants