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

feat(add-date-prefix-on-publish): implementation #571

Merged
merged 2 commits into from
Jan 12, 2019
Merged

feat(add-date-prefix-on-publish): implementation #571

merged 2 commits into from
Jan 12, 2019

Conversation

Geobert
Copy link
Contributor

@Geobert Geobert commented Dec 7, 2018

addresses #562

still need to figure out flag's name and it behaviour (see RFC)

@Geobert
Copy link
Contributor Author

Geobert commented Dec 7, 2018

It seems that rustfmt has changed again with 1.31, Shall I update Travis and appveyor to 1.31?

@epage
Copy link
Member

epage commented Dec 7, 2018

It seems that rustfmt has changed again with 1.31, Shall I update Travis and appveyor to 1.31?

Go for it. Also, I believe rustfmt.toml should now be stable, so let's see about locking down the config with that so we get more consistent behavior.

epage
epage reviewed Dec 7, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
epage
epage reviewed Dec 7, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
epage
epage reviewed Dec 7, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
epage
epage reviewed Dec 7, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
epage
epage reviewed Dec 7, 2018
View reviewed changes
src/cobalt_model/datetime.rs Outdated Show resolved Hide resolved
@Geobert
Copy link
Contributor Author

Geobert commented Dec 14, 2018

Not finished, wanted to check my CI settings and "backup" my code

@Geobert
Copy link
Contributor Author

Geobert commented Dec 28, 2018

@EDPAGE: Apart from this #571 (review) I think I'm done here :)

@Geobert
Copy link
Contributor Author

Geobert commented Dec 28, 2018 •
edited
Loading

tests are running fine on my computer… -_-

EDIT: wait, null.rs test are not running, oh some feature flag stuff I believe

@Geobert
Copy link
Contributor Author

Geobert commented Dec 28, 2018

@EDPAGE: this time, there's only #571 (review) left and then it will be ready

epage
epage reviewed Dec 28, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
epage
epage reviewed Dec 28, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
epage
epage reviewed Dec 28, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
@epage
Copy link
Member

epage commented Dec 28, 2018

I'm still a little torn on what the right design is for this.

Do we need a config flag?

  • The reason in favor of a config flag is this is a per-site and not per-user or per-run option

Should the config flag impact anything besides publish?

  • I have a bias against a config flag that is for anything besides cobalt build.
    • cargo handles this with a generic metadata table for external tools to have custom config in the same file

Maybe this is good enough. shrug

epage
epage reviewed Dec 28, 2018
View reviewed changes
src/bin/cobalt/new.rs Outdated Show resolved Hide resolved
@Geobert
Copy link
Contributor Author

Geobert commented Dec 28, 2018

The reason in favor of a config flag is this is a per-site and not per-user or per-run option

It's exactly why I prefer a config flag, it's not a per-run option, it's a choice on the filename that makes the website.

Should the config flag impact anything besides publish?

As it manages the published_date field, I think this is the job of publish only.

I have a bias against a config flag that is for anything besides cobalt build.

Why so? As publish is a built in function and I don't see why it would became an external tool (for now).

@Geobert
Copy link
Contributor Author

Geobert commented Jan 5, 2019

As far as I'm concern, this is ready to merge if you don't have any issue with it

@epage
Copy link
Member

epage commented Jan 5, 2019

Ah, you did a force-push, no wonder I didn't get a notification that there were changes.

Just the one open issue.

@dependabot-support
build(deps): bump toml from 0.4.9 to 0.4.10
e0e3cda 
Bumps [toml](https://github.com/alexcrichton/toml-rs) from 0.4.9 to 0.4.10.
- [Release notes](https://github.com/alexcrichton/toml-rs/releases)
- [Commits](toml-rs/toml-rs@0.4.9...0.4.10)

Signed-off-by: dependabot[bot] <support@dependabot.com>
@Geobert
Copy link
Contributor Author

Geobert commented Jan 5, 2019

Ah, you did a force-push, no wonder I didn't get a notification that there were changes.

I didn't know that ^^'

I'll mention you next time I force push ^^

epage
epage reviewed Jan 7, 2019
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
@epage
Copy link
Member

epage commented Jan 7, 2019

Geobert force-pushed the Geobert:publish-add-date-prefix branch from 15001b0 to 9b90af1

Randomly came on here just in case :)

@epage
Copy link
Member

epage commented Jan 12, 2019

I think this is in a state to clean up commit history + force-push so I can merge it.

feat(add-date-prefix-on-publish): implementation
19c3fee 
fmt

feat(add-date-prefix): apply comments

feat(add-date-prefix): breaking change, do not process date prefix by default anymore.

fix wrong rebase

fix test

fix test from null.rs

apply comment

restoring overdid refactor

bring back comment
@Geobert
Copy link
Contributor Author

Geobert commented Jan 12, 2019

Squash done! :D

@epage epage merged commit 95839ac into cobalt-org:master Jan 12, 2019
@Geobert Geobert deleted the publish-add-date-prefix branch January 13, 2019 17:44
@Geobert Geobert restored the publish-add-date-prefix branch January 13, 2019 17:47
@Geobert Geobert deleted the publish-add-date-prefix branch January 28, 2019 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Geobert @epage @dependabot-support