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

Fix description length validation and tag limiting #348

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

askmeaboutlo0m
Copy link
Contributor

@askmeaboutlo0m askmeaboutlo0m commented Jul 22, 2024

NOTE: This contains the stuff from #305, since I don't want to put work into fixing conflicts twice from first cherry-picking it out and then rebasing it again later. Either merge that other PR first or do only this one. Look at the individual commits to get a clean diff. Resolved.

The validation functions would get passed the raw description as entered by the user and would make some half-hearted effort to parse them into something sensible, but this completely breaks down in the face of shortcuts like {default}, username shortcuts that get turned into long links etc. Now those functions get the actually parsed description passed to them, the only unresolved shortcut in them being the {tags} one, since the length of that is variable and up to the validation function to deal with and tell the user if tags didn't fit.

Since #286, tags insertion is also pretty much busted entirely, since it runs at the wrong time, so it may insert too many tags and then cause an error down the line because the resulting description is too long. That insertion logic has now been moved to the end of the description processing, where it belongs.

@askmeaboutlo0m askmeaboutlo0m changed the base branch from master to develop July 22, 2024 11:17
@mvdicarlo
Copy link
Owner

I kind of want to re-evaluate tag insertions into descriptions.
I've slowly been souring on content injection through custom shortcuts that depend on other fields because it increases complexity. These might be better choices as checkboxes to append or prepend text. It may be more limiting but easier to work with.

The validation functions would get passed the raw description as entered
by the user and would make some half-hearted effort to parse them into
something sensible, but this completely breaks down in the face of
shortcuts like {default}, username shortcuts that get turned into long
links etc. Now those functions get the actually parsed description
passed to them, the only unresolved shortcut in them being the `{tags}`
one, since the length of that is variable and up to the validation
function to deal with and tell the user if tags didn't fit.

Since mvdicarlo#286, tags insertion is also pretty much busted entirely, since it
runs at the wrong time, so it may insert too many tags and then cause an
error down the line because the resulting description is too long. That
insertion logic has now been moved to the end of the description
processing, where it belongs.
@askmeaboutlo0m
Copy link
Contributor Author

I preferred the old approach of just appending tags anyway, it worked fine. I don't think that's directly related to this patch though, since the description length counting previously got all shortcuts wrong, {tags} is just a particularly egregious case.

@askmeaboutlo0m askmeaboutlo0m marked this pull request as draft July 24, 2024 16:53
@askmeaboutlo0m
Copy link
Contributor Author

Just ran into an issue where it somehow ended up miscounting the tags regardless. Turned this into a draft for now until I can figure that one out.

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