-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enable additional sources #6
Conversation
Allow to merge multiple collections source Allow to directly give a collection in the config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only blocking point is the changelog, as it does not abide by our rules and has a wrong reference.
There is room for improvement in code quality for the tests and the parsing algorithm, which mixes promise style and imperative style and is more complex than necessary. It would benefit from some refactoring. This is acceptable as is if we're short on time.
Overall, this change is an interesting case where, if we knew of deployed instances, it would be easy and relevant to have backward compatibility.
See also #4 (review) |
Co-authored-by: Matti Schneider <gh@mattischneider.fr>
Co-authored-by: Matti Schneider <gh@mattischneider.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment in the helper
file!
|
||
### Changed | ||
|
||
- **Breaking:** Support arbitrary collections as sources; rename `collectionsUrl` config entry into `collections` and wrap it in an array, refer to the [documentation](https://docs.opentermsarchive.org/api/federated/#configuring) for details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would not be safer to use a permalink (i.e. to the commit in the doc) rather than a doc URL that will not be valid in a few months.
First part of the review in #4