Skip to content

Convert Ascii into an enum. #14

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

Merged
merged 4 commits into from
Mar 28, 2016
Merged

Conversation

tormol
Copy link
Collaborator

@tormol tormol commented Mar 26, 2016

Allows static initialization, nicer pattern matches and comparisons.

Naming conventions:

  • alphabetic: just use it. breaks CamelCase convention, but worth it for simplicity.
  • digits: prepend _ to make them valid identifiers, but still short.
  • non-alphanumeric but visible: Use Wikipedia names CamelCased, but remove -Mark endings.
  • rarely used control codes: use uppercase acronym to deter use.
  • more commonly used control codes: Expand and CamelCase acronym to preserve meaning, eg LineFeed not NewLine or LineBreak.

I'm not certain which control codes should be expanded; Currently BEL and DEL are, but should they?

I'm not sure whether this is a breaking change.

@@ -52,13 +184,13 @@ impl Ascii {
/// Converts an ascii character into a `u8`.
#[inline]
pub fn as_byte(&self) -> u8 {
self.chr
unsafe{ transmute(*self) }
Copy link
Owner

Choose a reason for hiding this comment

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

This conversion doesn't need an unsafe block because *self as u8 works as expected.

@tomprogrammer
Copy link
Owner

I basically agree with the naming convention you chose, only minor discussion:

  • In my opinion it's more "rustic" to write Ascii::Nul instead of Ascii::NUL. I propose changing the acronyms to only uppercase the first character.
  • As you already mentioned it's difficult to categorize every control code by frequency of use. I guess that's not a severe question so if no discussion comes up I won't change the names.

The public interface isn't changed, in fact there is a new way to initialize Ascii. Therefore it isn't a breaking change. Nevertheless I will test that with crates depending on this crate, to be sure.

tormol added 4 commits March 28, 2016 06:18
Allows static initialization, nicer pattern matches and comparisons.

Naming conventions:

* alphabetic: just use it. breaks CamelCase convention, but worth it for simplicity.
* digits: prepend _ to make them valid identifiers, but still short.
* non-alphanumeric but visible: Use Wikipedia names CamelCased, but remove -Mark endings.
* rarely used control codes: use uppercase acronym to deter use.
* more commonly used control codes: Expand and CamelCase acronym to preserve meaning, eg LineFeed not NewLine or LFneBreak.

I'm not certain which control codes should be expanded; Currently BEL and DEL are, but should they?

I'm not sure whether this is a breaking change.
Control code names get expanded iff they have an escape code in [this table](.
Else the two-or-three letter uppercase code name is used.

)#	modified:   src/ascii.rs
@tormol
Copy link
Collaborator Author

tormol commented Mar 28, 2016

Rebased on master to use wrapping_sub. Also added rustdocfor each enum variant in the first commit.

I decided to categorize control codes on whether they have an escape code listed in this table

While not rustic, I think uppercase names prevent confusion:

  • is At '@' or some historical control code?
  • is Sub some historical control code or '-'?

@tomprogrammer
Copy link
Owner

I'm fine with your PR, thanks for your work!

I like the categorization based on the existence of an escape code, that makes it easier to guess how the variant is named. Your arguments also convinced me, that we should keep uppercase names for control codes. Indeed I've never seen control codes in CamelCase notation.

One minor point is that you used block comments /* */ for the description of the variants. I'd like to change this to rustdoc comments ///. Usually I look at the rustdoc documentation instead the plain code to find out how to use the library.

I can make that little change myself and thank your for your PR!

@tomprogrammer tomprogrammer merged commit 7b0556e into tomprogrammer:master Mar 28, 2016
@tomprogrammer
Copy link
Owner

Do you need the changes in this PR urgently? If so I will do a release, else I'll wait batching up some more changes I'm currently working on. ( #13 for example)

@tormol
Copy link
Collaborator Author

tormol commented Apr 5, 2016

No, I don't need the changes, and I'm working on another one.

/**...*/ is also a doc comment, I used them here to avoid two lines for every variant.

@tomprogrammer
Copy link
Owner

Oh, right, maybe I'll revert my changes that modify the doc-comments before the release.

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