Add named tuples and hex color tokens by reknih · Pull Request #1 · typst/typst

reknih · 2020-07-14T17:13:51Z

This Pull Request implements the specification for

named tuples like cmyk(52.6, 16, 80.4, 41.1)
and hexadecimal RGB(A) color literals, e.g. #20d82a

This pull request includes comprehensive test cases for the new parsing features.
The request should be merged to provide the language groundwork for color usage and other styling applications.

laurmaedje

I added some thoughts and comments, but I think this is already pretty good :)

laurmaedje · 2020-07-14T21:40:20Z

src/syntax/expr.rs

 use std::fmt::{self, Write, Debug, Formatter};
 use std::iter::FromIterator;
+use std::ops::Deref;
+use std::u8;


Do you need that use std::u8; import? I think u8::from_str_radix is associated with the type, not the module.

Does not seem to work without the import, so it has to stay :/

laurmaedje · 2020-07-14T21:41:41Z

src/syntax/expr.rs

+        let permissable = &[3, 4, 6, 8];
+
+        if !permissable.contains(&len) {
+            return None;
+        }
+


I would suggest adding another let short = len == 3 || len == 4; and then checking long || short and returning None if that is false instead of the slice-contains.

laurmaedje · 2020-07-14T21:43:44Z

src/syntax/expr.rs

+            let pos = elem * item_len;
+
+            if let Ok(val) = u8::from_str_radix(
+                &hex_str[pos..(pos+item_len)], 16) {


I think this can panic if hex_str turns out not to be ASCII if you index into a UTF-8 char. I suggest adding a hex_str.is_ascii() check at the start of the method.

laurmaedje · 2020-07-14T21:46:13Z

src/syntax/expr.rs

+            let item_len = if long { 2 } else { 1 };
+            let pos = elem * item_len;
+
+            if let Ok(val) = u8::from_str_radix(


This might be written more concisely as

let item = &hex_str[pos..(pos+item_len)]; values[elem] = u8::from_str_radix(item, 16).ok()?;

Done as well.

laurmaedje · 2020-07-14T21:48:07Z

src/syntax/expr.rs

+/// ```
+#[derive(Clone, PartialEq)]
+pub struct NamedTuple {
+    pub name: Spanned<Ident>,


Maybe add a doc-comment, even though I guess it's pretty obvious.

laurmaedje · 2020-07-14T21:48:41Z

src/syntax/expr.rs

+impl Debug for NamedTuple {
+    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
+        f.debug_struct("named tuple")
+            .field("name", &self.name)
+            .field("values", &self.tuple)
+            .finish()
+    }
+}


This is pretty close to the output of derive(Debug). In that case, I think deriving would be simpler.

Keeping it DRY. Smart.

laurmaedje · 2020-07-14T21:51:36Z

src/syntax/parsing.rs

+                let color_opt = RgbaColor::from_str(s);
+
+                if let Some(color) = color_opt {


Maybe write that RgbaColor::from_str(s) directly into the if let.

laurmaedje · 2020-07-14T21:53:11Z

src/syntax/parsing.rs

+                } else {
+                    // Heal color by assuming black
+                    self.feedback.errors.push(err!(first.span;
+                        "expected valid color, found invalid color #{}", s));


I think adding the error message & healing here is a good idea, but the message might be slightly redundant. What do you think about adding it here, but keeping just "invalid color"?

I have also introduced a healed flag on RgbaColor that indicates whether the color is a fallback, so that other methods can react more smartly (or not at all).

laurmaedje · 2020-07-14T21:55:26Z

src/syntax/parsing.rs

+        let start = name.span.start.clone();
+        let end = tuple.span.end.clone();


Position should implement Copy, so I don't think you need to clone. But, in fact, there is also a convenience method Span::merge(name.span, tuple.span), which you can use here.

Googled up on the difference between Clone and Copy traits in general and copy seems like the Right Thing To Do:tm: here,

laurmaedje · 2020-07-14T22:03:25Z

src/syntax/tokens.rs

+
+                // This may be a hex expression
+                let hex_expr = if c == '#' && !body {
+                    let payload = self.read_string_until(|n| {
+                        match n {
+                            '0'..='9' | 'a'..='f' | 'A'..='F' => false,
+                            _ => true,
+                        }
+                    }, false, 0, 0).0;
+
+                    Some(ExprHex(payload))
+                } else {
+                    None
+                };
+


Since this starts with a clearly defined character (#), you should be able to write it earlier in match instead of writing it in the fallback case. For example, right after the string case á la

'#' if self.mode == Header => self.parse_hex_value(),

and then factor the code out into that method. Also, I just noticed that there is a function in the standard library, so you can just do it like this:

ExprHex(self.read_string_until(|n| !n.is_digit(16), false, 0, 0).0)

There is also the thing that stopping when there is no hex digit might lead to a bit counter-intuitive error messages:

[page: background=#55z555] ^^^^ unexpected identifier

Where on might expect:

[page: background=#55z555] ^^^^^^^ invalid color

But I'm also not sure how important that is and what the best way would be here. Maybe reading until there is a non-alphanumeric character would also be an option.

Okay, it will now do is_ascii_alphanumeric in the tokenizer, that will hopefully create better error messages.

reknih · 2020-07-15T18:58:21Z

The new commit with the feedback incorporated is now pushed.

Resolves typst#1

Multi-container apps: add workaround for non amd64 machines

* feat: mod typst * chore: bump version * Avoid panic if a field doesn't have name --------- Co-authored-by: ParaN3xus <paran3xus007@gmail.com> Co-authored-by: ParaN3xus <136563585+ParaN3xus@users.noreply.github.com>

Add named tuples and hex color tokens

28c3a79

reknih requested a review from laurmaedje July 14, 2020 17:13

laurmaedje reviewed Jul 14, 2020

View reviewed changes

Healed field for RgbaColors, CodeRev feedback 🤝

1683ca8

Use FromStr trait and formatting ✨

e96f383

laurmaedje merged commit 0fd327b into master Jul 15, 2020

laurmaedje deleted the named_tuples branch July 15, 2020 21:49

Enivex mentioned this pull request Apr 3, 2023

Braket notation, set notation, and middle delimeters #240

Closed

maxcrees mentioned this pull request Apr 21, 2023

Format number as string with an explicit base #918

Closed

kan190729 pushed a commit to oss-experiment-uec/2024-k2210165-typst that referenced this pull request Dec 23, 2024

Remove unused ARG from Dockerfile

e6538ef

Resolves typst#1

kan190729 pushed a commit to oss-experiment-uec/2024-k2210165-typst that referenced this pull request Dec 23, 2024

Merge pull request typst#1 from antalvarenga/antalvarenga-patch-1

64a1666

Multi-container apps: add workaround for non amd64 machines

damienflament mentioned this pull request Jul 5, 2025

Equality between contents containing raw text fails #6559

Closed

uwni mentioned this pull request Feb 3, 2026

Glitch when resize accent #7796

Open

1 task

		let color_opt = RgbaColor::from_str(s);

		if let Some(color) = color_opt {

		let start = name.span.start.clone();
		let end = tuple.span.end.clone();

Uh oh!

Conversation

reknih commented Jul 14, 2020

Uh oh!

laurmaedje left a comment

Choose a reason for hiding this comment

Uh oh!

laurmaedje Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reknih commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

laurmaedje Jul 14, 2020 •

edited

Loading