Add --combined option #37

dylni · 2020-05-13T14:51:43Z

Thanks for creating this very useful crate.

This adds a --combined option that works the same way as not adding the option, except all tables are combined into a single table. It can create smaller tables than --enum when it's only necessary to check if a code point is part of any variant.

It also supports related categories, which --enum doesn't.

thomcc

This doesn't feel very useful to me. The only one of these that has meaningful output at all is the SCRIPT table, and that's because we don't include Unknown/Zzzz in there (which might be a bug, IDK) -- The tables are &[(0, 0x10ffff)]. I guess maybe the idea is you could use it with --exclude / --include? Anyway, @BurntSushi's call.

If we're going to add it, it should be supported for all the other cases where we currently support --enum, probably...

Just in terms of the code itself I think it would be better if it tried harder to use the existing output functions in writer.rs.

I also suspect it would be better if the flag were on the writer.opts structure...

thomcc · 2020-05-13T17:59:11Z

src/writer.rs

@@ -417,6 +458,29 @@ impl Writer {
        Ok(())
    }

+    fn range_set_slice(


I think this function is the same as ranges_slice.

thomcc · 2020-05-13T17:59:23Z

src/writer.rs

+    /// Write a set that contains ranges of codepoints.
+    ///
+    /// The smallest numeric type is used when applicable.
+    pub fn range_set(&mut self, name: &str, set: &[u32]) -> Result<()> {


I think if you just turn the slice into BTreeSet instead you could use the existing ranges function here. This would make supporting trie output easy, as well as the --split-ranges option in #35, for example.

Actually supporting these might make the help text for the flags confusing though...

thomcc · 2020-05-13T18:12:29Z

src/app.rs

@@ -221,6 +221,11 @@ pub fn app() -> App<'static, 'static> {
        .arg(Arg::with_name("rust-enum").long("rust-enum").help(
            "Emit a Rust enum and a table that maps codepoints to bidi class.",
        ))
+        .arg(
+            Arg::with_name("combined").long("combined").help(


It's better if you put this up with the other args and just clone it in. I don't know why --enum and --rust-enum don't, but they also have slightly varying help text for each one, which is probably why.

That makes sense. I was using --rust-enum as a reference.

dylni · 2020-05-13T19:01:28Z

This doesn't feel very useful to me. The only one of these that has meaningful output at all is the SCRIPT table

I would only be using this for the general-category subcommand, which does have useful output. The others were only added for compatibility, but I can remove them if they don't make sense.

Just in terms of the code itself I think it would be better if it tried harder to use the existing output functions in writer.rs.

You're right. I didn't expect these functions to exist. It's much better to use them.

I also suspect it would be better if the flag were on the writer.opts structure...

I don't think this would make sense, but you know the code better than I do. This option is similar to --enum and --rust-enum, which also aren't on the structure. It also conflicts with both of those options.

thomcc · 2020-05-13T19:10:17Z

Hmm... For a while I've wanted the ability to generate a custom table based on a combination of property bools / property values. For example, perl-word is an example of a custom table like this, as are a lot of things I've wanted.

Anyway, I'm going to file a bug for adding something like that -- it would have solved your use case, and some of mine. I think there are still open questions around that though, so I'm not saying that filing the bug makes this obsolete.

BurntSushi · 2020-11-18T18:51:18Z

@dylni This looks like a simple addition so I'm inclined to accept it if it's useful, but I don't quite understand the use case. Can you say more about why this is useful to you? It sounds like you want to use it with general-categry, but isn't every codepoint in exactly one general category? So how does this help you?

dylni · 2020-11-18T20:12:35Z

@BurntSushi Yes, I use it to generate this file. That crate only needs to check if characters belong to one of the "other" or "separator" categories, so searching one table is easier and more efficient.

If you remove the --combined option from the command, 3 tables are generated instead of 1, which is not optimal for my use case.

It sounds like you want to use it with general-categry, but isn't every codepoint in exactly one general category? So how does this help you?

Yes, I could search both tables, and that would very likely have a negligible impact on performance. However, since a smaller and simpler table can be generated, that is what I would prefer to include in my crate.

BurntSushi · 2020-11-18T22:18:56Z

@dylni Ah I see. I think the thing missing from your explanation is that you were only generating a subset of general category tables via the --include flag.

Can you add a little extra docs to the --combined flag here saying why someone might want to use it? Basically just a couple sentences from your comments here.

dylni · 2020-11-19T22:18:47Z

@BurntSushi

Ah I see. I think the thing missing from your explanation is that you were only generating a subset of general category tables via the --include flag.

Oh, I can see how this seems very strange without that context.

Can you add a little extra docs to the --combined flag here saying why someone might want to use it? Basically just a couple sentences from your comments here.

Sure, the last commit extends the documentation.

BurntSushi · 2020-11-22T14:58:01Z

src/app.rs

+        "Emit a single table with all included codepoint ranges. You might \
+        want to use this option when checking if characters belong to a \
+        subset of categories, since only one table will need to be checked. \
+        Searching the combined table should be simpler and more efficient.",


Excellent. Thank you!

BurntSushi

Thanks!

BurntSushi · 2020-11-22T15:04:47Z

This PR is on crates.io in ucd-generate 0.2.9.

dylni · 2020-11-22T16:47:00Z

Thanks! It's working very well

dylni added 2 commits May 13, 2020 10:39

writer: Add methods for --combined option

c5aa590

cli: Add --combined option

e613ae6

thomcc reviewed May 13, 2020

View reviewed changes

dylni added 2 commits May 13, 2020 14:57

style: Avoid repeating arg

83c711f

style: Use existing methods

e1e855a

thomcc mentioned this pull request May 13, 2020

Subcommand to generate custom tables containing a combination of properties / property values. #38

Open

Extend documentation

df40626

BurntSushi reviewed Nov 22, 2020

View reviewed changes

BurntSushi approved these changes Nov 22, 2020

View reviewed changes

BurntSushi merged commit 957d038 into BurntSushi:master Nov 22, 2020

dylni deleted the add-combined-option branch November 22, 2020 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --combined option #37

Add --combined option #37

dylni commented May 13, 2020

thomcc left a comment

thomcc May 13, 2020

dylni May 13, 2020

thomcc May 13, 2020

dylni May 13, 2020

thomcc May 13, 2020

dylni May 13, 2020 •

edited

Loading

dylni commented May 13, 2020

thomcc commented May 13, 2020

BurntSushi commented Nov 18, 2020

dylni commented Nov 18, 2020

BurntSushi commented Nov 18, 2020

dylni commented Nov 19, 2020

BurntSushi Nov 22, 2020

BurntSushi left a comment

BurntSushi commented Nov 22, 2020

dylni commented Nov 22, 2020

Add --combined option #37

Add --combined option #37

Conversation

dylni commented May 13, 2020

thomcc left a comment

Choose a reason for hiding this comment

thomcc May 13, 2020

Choose a reason for hiding this comment

dylni May 13, 2020

Choose a reason for hiding this comment

thomcc May 13, 2020

Choose a reason for hiding this comment

dylni May 13, 2020

Choose a reason for hiding this comment

thomcc May 13, 2020

Choose a reason for hiding this comment

dylni May 13, 2020 • edited Loading

Choose a reason for hiding this comment

dylni commented May 13, 2020

thomcc commented May 13, 2020

BurntSushi commented Nov 18, 2020

dylni commented Nov 18, 2020

BurntSushi commented Nov 18, 2020

dylni commented Nov 19, 2020

BurntSushi Nov 22, 2020

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

BurntSushi commented Nov 22, 2020

dylni commented Nov 22, 2020

dylni May 13, 2020 •

edited

Loading