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

Fixed example codes (syntax) #3730

Merged
merged 1 commit into from Dec 20, 2016
Merged

Conversation

maiha
Copy link
Contributor

@maiha maiha commented Dec 18, 2016

It seems that some of commented codes are wrong or out of date.
I tried to compile all 1088 examples, and fixed syntax failures as possible.

master

  • success: 894, failure: 201

PR

  • success: 969, failure: 119

There are still failures where the problem is semantics level such as

  • insufficient code (using stub objects)
  • abstract code
  • runtime executions like macro

I'll create another PR about this when I have a chance.

Thanks.

#
# Levenshtein.distance("algorithm", "altruistic") # => 6
# Levenshtein.distance("hello", "hallo") # => 1
# Levenshtein.distance("こんにちは", "こんちは") # => 1
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong indent

# NamedTuple(foo: String, bar: Int64).from({:foo => "world", :bar => 2}).class # => {foo: String, bar: Int64}
# NamedTuple(foo: String, bar: Int32).from({:foo => "world", :bar => 2}) # => {foo: "world", bar: 2}
# NamedTuple(foo: String, bar: Int32).from({"foo" => "world", "bar" => 2}) # => {foo: "world", bar: 2}
# NamedTuple(foo: String, bar: Int32).from({:foo => "world", :bar => 2}).class # => {foo: String, bar: Int32}
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing Int64 to Int32 for bar disables casting feature which this example is about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, the casting feature is already broken in 0.20.1.
So I assumed the spec has been changed.
I'll revert it soon. Then, would I create the issue?

@asterite
Copy link
Member

We could probably have a way to let the doc generator compile and run these small examples, even checking if they return the correct values. So something similar to doctest. I believe this is the correct solution so examples don't become outdated.

#
# Levenshtein.distance("algorithm", "altruistic") # => 6
# Levenshtein.distance("hello", "hallo") # => 1
# Levenshtein.distance("こんにちは", "こんちは") # => 1
Copy link
Contributor

Choose a reason for hiding this comment

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

still wrong indent

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sija I'm pretty sure this might end up being screwy no matter what; the Unicode symbols used are variable-width, so different fonts are all going to show it a bit differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@kirbyfan64 hmm, maybe it's a formatter bug?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to be a formatter bug. I'll fix it soon :-)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it's not a bug. Japanese characters have a longer width, so it pushes the comment to the right...

@RX14
Copy link
Contributor

RX14 commented Dec 18, 2016

@asterite Having examples compile by default is really nice, because it opens the path for a possible future PR making doc examples interactive in the browser. Will probably have to wait until we get a wasm or JS compiler target however.

Unfortunately having every example compile would reduce brevity, so there should probably be an option to turn this off example-by-example. Or maybe there could be a way to hide parts of the example so you could create examples which compile, but only show the non-boilerplate code.

In addition, I would make testing the doc examples part of the spec runner, because having a piece of code and an expected result makes it a specification. However, if you want to keep it part of crystal doc for simplicity I understand.

@asterite
Copy link
Member

@RX14 I'm trying to compile every code example right now, and it's taking forever. I agree with all of your points and I have the same concerns too. I think it will probably remain as an idea, at least for some time :-)

@maiha
Copy link
Contributor Author

maiha commented Dec 18, 2016

Forgot to run crystal tool format. re-pushed.

@RX14
Copy link
Contributor

RX14 commented Dec 19, 2016

@asterite one thing we could do now is add an option to dump the example code to a directory. This would make extracting the example code when we do want to manually check it easier.

@asterite
Copy link
Member

@maiha Thank you so much for this! Keeping example code up to date and compiling is super important!

@asterite asterite merged commit 19abbfc into crystal-lang:master Dec 20, 2016
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

5 participants