Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
U-Blad Corpus #1590
U-Blad Corpus #1590
Changes from 10 commits
c4a35c2
eac1369
e4d4df0
6c5e023
2478951
cd29f43
0dc2659
136d325
633be4d
c4e1e08
c42aa8c
1850050
40a6ca7
4432df9
fd1d4df
9dff0d8
212ed79
78d64bc
4333b44
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Leuke afbeelding, nice.
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.
Since the corpus is imported when starting up the server, this may have unintended side effects. I would recommend restoring the locale setting when it's no longer needed.
It's not clear from the code when that would be, by the way; please document what this setting is intended to achieve.
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 agree with Luka, this should not live in runtime code. My guess it is used in indexing?
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.
It is used for the indexing of the date; where would I put it then? Or what should I restore the locale to exactly?
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.
Neat!
Since the return value is just a node with the string content you want, why not use
extract_soup_func
to return the string directly?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.
Why do these attributes need to be configurable?
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.
scan_image_type
is used inGetMediaView
to open the image type correctly,allow_image_download
is used in the frontend to allow downlaods and the default isfalse
. Or am I missing something?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.
Yes, the values make sense, but you could just write the value directly, i.e.
This definition allows the values to be configured in
settings
, but why? That would make sense if, for instance, image download would need to be disabled in some environments but not in others.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.
These fields are missing
es_mapping=keyword_mapping()
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.
es_mapping=int_mapping()
, I think?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.
keyword_mapping cause sometimes it is a phrase instead of a number