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.

Sign up for GitHub

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

Jump to bottom

fix example codes (2018-06) #6315

Merged
merged 2 commits into from Aug 4, 2018
Merged

fix example codes (2018-06) #6315

merged 2 commits into from Aug 4, 2018

Conversation

maiha
Copy link
Contributor

@maiha maiha commented Jul 2, 2018

Congrats 0.25.1! And fixed example codes.

0.25.1 [b782738]

Specs 152 (117 successes, 14 failures, 21 pending)
Examples 1409 (1047 successes, 141 failures, 221 pending)

PR

Specs 152 (131 successes, 0 failures, 21 pending)
Examples 1409 (1153 successes, 0 failures, 256 pending)

Best regards,

straight-shoota
straight-shoota reviewed Jul 2, 2018
View reviewed changes
src/http/common.cr Outdated
@@ -246,7 +246,7 @@ module HTTP
# timezone `GMT` (interpreted as `UTC`).
#
# ```
# HTTP.format_time(Time.new(2016, 2, 15)) # => "Sun, 14 Feb 2016 21:00:00 GMT"
# HTTP.format_time(Time.new(2016, 2, 15)) # => "Mon, 15 Feb 2016 00:00:00 GMT"
Copy link
Member

Choose a reason for hiding this comment

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

The output depends on local time zone configuration. Time.new should better receive a specific time zone.

src/time.cr Outdated
# time = Time.new(2016, 2, 15, 10, 20, 30, location: Time::Location.load("Europe/Berlin"))
# time.to_s # => 2016-02-15 10:20:30 +01:00 Europe/Berlin
# The time-of-day can be omitted and defaults to midnight (start of day):
# time # => 2016-02-15 10:20:30 +01:00 Europe/Berlin
Copy link
Member

Choose a reason for hiding this comment

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

Please add .to_s and wrap result in quotes like in the other examples.

src/time.cr Outdated
@@ -907,7 +907,7 @@ struct Time
# Format this time using the format specified by [RFC 2822](https://www.ietf.org/rfc/rfc2822.txt).
#
# ```
# Time.new(2016, 2, 15).to_rfc2822 # => "Mon, 15 Feb 2016 00:00:00 -0400"
# Time.new(2016, 2, 15).to_rfc2822 # => "Mon, 15 Feb 2016 00:00:00 +0000"
Copy link
Member

Choose a reason for hiding this comment

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

Example should use a specific time zone.

src/time/format/custom/http_date.cr Outdated
@@ -12,7 +12,7 @@ require "./rfc_2822"
# Time::Format::HTTP_DATE.parse("Sunday, 14-Feb-16 21:00:00 GMT") # => 2016-02-14 21:00:00 UTC
# Time::Format::HTTP_DATE.parse("Sun Feb 14 21:00:00 2016") # => 2016-02-14 21:00:00 UTC
#
# Time::Format::HTTP_DATE.format(Time.new(2016, 2, 15)) # => "Sun, 14 Feb 2016 21:00:00 GMT"
# Time::Format::HTTP_DATE.format(Time.new(2016, 2, 15)) # => "Mon, 15 Feb 2016 00:00:00 GMT"
Copy link
Member

Choose a reason for hiding this comment

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

Example should use a specific time zone.

src/time.cr Outdated
@@ -873,7 +873,7 @@ struct Time
# Format this time using the format specified by [RFC 3339](https://tools.ietf.org/html/rfc3339) ([ISO 8601](http://xml.coverpages.org/ISO-FDIS-8601.pdf) profile).
#
# ```
# Time.new(2016, 2, 15).to_rfc3339 # => "2016-02-15T00:00:00+00:00"
# Time.new(2016, 2, 15).to_rfc3339 # => "2016-02-15T00:00:00Z"
Copy link
Member

Choose a reason for hiding this comment

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

Example should use a specific time zone.

@maiha
Copy link
Contributor Author

maiha commented Jul 2, 2018

@straight-shoota thanks. I added timezone by using Time.utc rather than Time.new.

@asterite
Copy link
Member

asterite commented Jul 2, 2018

@maiha Nice! How long does it take to compile and run all examples? Do you generate a single big program, or is it one compilation per example?

@maiha
Copy link
Contributor Author

maiha commented Jul 2, 2018

@asterite

  • time: 8 mins (Intel 3GHz)
  • spec: one spec file per one class file, because
    • putting all examples into 1 spec file: failed due to name conflictions about variables and classes. And it takes a long time to run again.
    • putting 1 example into 1 spec file: failed because some examples uses variables and classes defined in previous examples.

For example, this is a generated spec file from src/array.cr . And I run it in docker for security reason.
https://github.com/maiha/crystal-examples/blob/master/gen/spec/array_spec.cr

@maiha
Copy link
Contributor Author

maiha commented Jul 2, 2018

I forgot to run tool format.

@asterite
Copy link
Member

asterite commented Jul 2, 2018

8 minutes is a lot of time, but I guess for smaller projects it will be a reasonable time.

I wonder if we should embed this feature directly in the compiler. That way we'll have doctest embedded in the language. And we'll be able to run this in CI to check that all examples compile/run fine. And they never get out of date.

I think the CI already takes a lot of time, so 8 minutes more isn't thaaat bad :-P

Thoughts?

@j8r
Copy link
Contributor

j8r commented Jul 2, 2018

It's not needed to add on the CI, because this is only about comments at the end, not code. Having some incorrect docs doesn't prevent to have a working crystal compiler.
However running it before each Crystal release will enforce consistent docs 👍

@maiha
Copy link
Contributor Author

maiha commented Jul 2, 2018

Execution time can be reduced by caching. So I think it is not a big problem.

As @j8r is worried, it is a problem that there is a not working code case. Actually, there are many image codes like the following.

It takes time manually to judge this. And Most of the time is spent here.😱
I think this is a major barrier in the case of automation.

@straight-shoota
Copy link
Member

@j8r It's not strictly necessary, but really nice to have if the code examples are automatically validated. This doesn't even need to be integrated in the compiler. We could run the examples checker right now in CI.

@maiha Sure, some examples need manual intervention, but that's what this is about. Having automatic checks in CI would prevent simple errors and encourage people to write working examples.

Obviously, not every example can be written in a way that it is actually executable. But it should be quite easy to flag such examples as ignore. For example using a special syntax highlight declaration: ```crystal doctest=ignore

@sdogruyol
Copy link
Member

@maiha this need a rebase and good to go 👍

@maiha
Copy link
Contributor Author

maiha commented Aug 4, 2018

rebased. I hope this will be merged before another conflict occurs. 😃

@bcardiff bcardiff added this to the 0.26.0 milestone Aug 4, 2018
@bcardiff bcardiff merged commit 6f8d7aa into crystal-lang:master Aug 4, 2018
@bcardiff
Copy link
Member

bcardiff commented Aug 4, 2018

Thanks @maiha

@straight-shoota straight-shoota mentioned this pull request Nov 26, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota left review comments

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:docs
Projects
None yet
Milestone
0.26.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@maiha @asterite @j8r @straight-shoota @sdogruyol @bcardiff @RX14