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

Add "chrono" feature #3

Merged
merged 1 commit into from Jan 6, 2020
Merged

Add "chrono" feature #3

merged 1 commit into from Jan 6, 2020

Conversation

Klok-e
Copy link
Contributor

@Klok-e Klok-e commented Dec 30, 2019

rust-oracle crate provides a feature that adds various trait implementations for the chrono crate that are particularly useful when working with dates.
This PR adds chrono feature that enables the chrono feature in rust-oracle

@rursprung
Copy link
Owner

thanks for this PR!

would it be possible for you to add the following things?

  • update README.md and document the new flag
  • update the rustdoc in lib.rs and document the new flag (probably more-or-less the same text as in the README)
  • update .travis.yml and turn it into a build matrix where one build is with the feature and the other without

with that the feature will be complete and easily visible for everyone.

@rursprung rursprung self-requested a review December 30, 2019 17:35
Copy link
Owner

@rursprung rursprung left a comment

Choose a reason for hiding this comment

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

see previous comment

@Klok-e
Copy link
Contributor Author

Klok-e commented Dec 30, 2019

Hey, I'd like to receive some help with .travis.yml. I added an env variable specifying chrono feature but the CI build fails as if the feature doesn't exist at all.

@Klok-e
Copy link
Contributor Author

Klok-e commented Dec 30, 2019

This is the error message:

error: Package `r2d2-oracle v0.2.0 (/home/travis/build/LokiVKlokeNaAndoke/r2d2-oracle)` does not have these features: `"chrono"`

@Klok-e
Copy link
Contributor Author

Klok-e commented Dec 31, 2019

Nevermind, fixed it myself. The solution has a lot of repetition though, couldn't think of a way to avoid it.

@Klok-e Klok-e requested a review from rursprung January 1, 2020 14:40
.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
.travis.yml Outdated
@@ -3,11 +3,14 @@ rust:
- stable
- 1.38.0 # lowest rust release against which we guarantee compatibility.
cache: cargo
env:
- CHRONO='--features "chrono"'
Copy link
Owner

Choose a reason for hiding this comment

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

this isn't working because you quotes "chrono". you can test this yourself on a local shell by doing the same as travis-ci:

$ export CHRONO='--features "chrono"'
$ cargo build $CHRONO
error: Package `r2d2-oracle v0.2.0 (Z:\development\r2d2-oracle)` does not have these features: `"chrono"`
$ export CHRONO='--features chrono'
$ cargo build $CHRONO
    Finished dev [unoptimized + debuginfo] target(s) in 1.32s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quotes worked on my machine (win 10) and that's how it's specified in the book. Maybe linux is different in that

Copy link
Owner

@rursprung rursprung left a comment

Choose a reason for hiding this comment

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

thanks a lot and sorry for the back-and-forth! i believe i now know how to get the travis build working as intended (with your original version of it). can you please do that change and also squash your commits so that there's a single commit containing the whole feature? otherwise i can also take care of that for you while merging if you want.

.travis.yml Outdated Show resolved Hide resolved
@Klok-e
Copy link
Contributor Author

Klok-e commented Jan 5, 2020

Is it ok now?

@Klok-e
Copy link
Contributor Author

Klok-e commented Jan 5, 2020

Fixed the commit message

Copy link
Owner

@rursprung rursprung left a comment

Choose a reason for hiding this comment

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

thanks a lot!

- cargo test --verbose
- cargo clippy --verbose
- cargo fmt && [ `git status -s | wc -l` -eq 0 ] # check that all files are formatted with `cargo fmt`
- cargo fmt && [ `git status -s | wc -l` -eq 0 ] # check that all files are formatted with `cargo fmt`
Copy link
Owner

Choose a reason for hiding this comment

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

nit-picking: this is removing the final linebreak and wouldn't have been necessary... but we can keep it as-is, doesn't hurt, just doesn't look 100% nice.

@rursprung
Copy link
Owner

i'll release this as a new version (0.3.0) hopefully tonight.

@rursprung rursprung closed this Jan 6, 2020
@rursprung rursprung reopened this Jan 6, 2020
@rursprung rursprung merged commit d92ebeb into rursprung:master Jan 6, 2020
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