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

Split encoding::types into encoding-types crate #89

Merged
merged 1 commit into from
Jul 21, 2015

Conversation

nagisa
Copy link
Contributor

@nagisa nagisa commented Jul 19, 2015

This allows for creation of alternative non-WHATWG encodings that use the same
interface as encodings defined in this crate without pulling in all the
tables and encodings.

This commit does not introduce any breaking changes; all the types previously defined in
encoding::types are reexported.

Fixes #81.


I tried to avoid breaking changes for now, but IMO fn decode being in encoding::types makes little sense; I’d move it to encoding at some point later.

@lifthrasiir lifthrasiir self-assigned this Jul 20, 2015
@lifthrasiir
Copy link
Owner

It looks mostly fine to me, with the following caveats:

  • I agree that encoding::types::decode is in the strange location (it was a result of aggressive module split, sigh), but we need to wait until 0.3 to get rid of it. You may want to move it into the crate root and reexport it from encoding::types.
  • I guess most (and in fact all, but I haven't tried that) types tests should be actually in a new crate.

for ch in string.chars() {
match ch {
'a'...'z' => { output.write_char(rotate_byte(ch as u8) as char) },
'A'...'Z' => { output.write_char(rotate_byte(ch as u8) as char) },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this split intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ROT13 is defined to only work on basic latin characters (a to z and A to Z). In ASCII table uppercase and lowercase letters are not adjacent, therefore the split.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that 'a'...'z' | 'A'...'Z' would probably work.

@nagisa
Copy link
Contributor Author

nagisa commented Jul 20, 2015

I guess most (and in fact all, but I haven't tried that) types tests should be actually in a new crate.

The tests depend on encoding::util which is a private module, therefore moving tests without rewriting them to not use index_iter is not possible. Do you still want to move them? I’ll change tests to not use index_iter then.

This allows for creation of alternative non-WHATWG encodings that use the same
interface as encodings defined in this crate without pulling in all the
tables and encodings.

This commit does not introduce any breaking changes; all the types previously defined in
encoding::types are reexported.

Fixes lifthrasiir#81.
lifthrasiir added a commit that referenced this pull request Jul 21, 2015
Split encoding::types into encoding-types crate
@lifthrasiir lifthrasiir merged commit 039c97c into lifthrasiir:master Jul 21, 2015
@lifthrasiir
Copy link
Owner

@nagisa Ah, that's a legitimate reason (and my suggestion can be incrementally implemented anyway). I have no reason not to merge this, so I went ahead; thank you for the PR!

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.

None yet

2 participants