symfony-cmf / seo-bundle

A SEO Solution for duplicate contents, page titles, etc.
https://cmf.symfony.com
47 stars 27 forks source link

Depth information in sitemap into 1.2 #284

Closed ElectricMaxxx closed 8 years ago

ElectricMaxxx commented 8 years ago

282 done on branch 1.2 to create a new 1.3 version out of it.

wouterj commented 8 years ago

I don't really get why this is on 1.2. Are we going to add a new feature in 1.2.1? Is this feature of such importance or is it a bug fixing feature?

If we want to release it as 1.3, we have to create a 1.3 branch from the 1.2 branch now and then target this PR to the 1.3 branch. Otherwise, this feature ends up in the 1.2 branch.

ElectricMaxxx commented 8 years ago

@WouterJ Reason: @dbu mentioned on the ticket, that the change of the index.html.twig is a BC break. When you don't agree i can live with 1.2.1.

dbu commented 8 years ago

i don't think we have a BC break here. but its a new feature. so strictly speaking it should be 1.3 (or 2.0, but we said doing a 1.3 soonish with non-breaking changes would be ok).

indeed wouter is right, the branch should be called 1.3, as 1.2 is for maintenance of the 1.2 version.

ElectricMaxxx commented 8 years ago

@dbu i do not understand what i have to change on the PR after your discussion.

btw: travis has a licence header style problem on this branch too :-)

wouterj commented 8 years ago

but we said doing a 1.3 soonish with non-breaking changes would be ok

Isn't it a bit strange to release a new minor with just one new feature?

dbu commented 8 years ago
but we said doing a 1.3 soonish with non-breaking changes would be ok

Isn't it a bit strange to release a new minor with just one new feature?

i don't see a problem releasing it.

i would propose to squash the commits, rebase on 1.2 and then create the 1.3 branch from 1.2 and open a new PR of this branch to 1.3. but if wouter is unhappy with that, we can either smuggle it into 1.2.1 or merge it into master and wait until 2.0 with releasing.

regarding styleci: i only did the fix on master. now done it on the 1.2 branch too and pushed. you will need to rebase this branch on 1.2 to get them in.

ElectricMaxxx commented 8 years ago

i rebased on 1.2 and squashed all commits.

dbu commented 8 years ago

styleci looks happy now \o/

@WouterJ how do you see this question of which release this can go into?

wouterj commented 8 years ago

Definitely not 1.2.1 imo. I don't care much about 1.3 vs 2.0. (please note that this PR is targetting 1.2 so it should not be merged)

ElectricMaxxx commented 8 years ago

@WouterJ it is should be merged into 1.2 and 1.3 into master later, cause maste have got changes after release.

dbu commented 8 years ago

if we want the new feature to correctly only go to a 1.3 branch, it needs to be merged into a 1.3 branch that we create from the 1.2 branch. and then we can merge 1.3 into master once this is merged to 1.3.

ElectricMaxxx commented 8 years ago

To get the stuff a clear in my head (and in the one of git too g):

right?

dbu commented 8 years ago

mostly. i would not release 1.3 right away, but wait a few weeks if something else comes up. we could also use the 1.3 branch to start deprecating things as we refactor in master. and target mid may for the 1.3 release. wdyt?

ElectricMaxxx commented 8 years ago

sounds like a plan

ElectricMaxxx commented 8 years ago

@rdohms you told me yesterday, that i can make bug fixes and really small stuff on the patch part of a semver version. When is a feature/change to big that i need a new minor version? We have an open diskussion on my PR in here. The feature is just an information, that i missed while implementing the sitemap feaature for version 1.2 - the depth. This is created and passed to the template (default one, mostly overwritten) now. there the change is minimal to, just one class entry, which does nothing when there is no css styling for that. I don't wanna wait for that change until our next release (major this time) is there, cause i have got an open PR on our website repository..

ElectricMaxxx commented 8 years ago

No answer? so i will do as proposed by @dbu

dbu commented 8 years ago

i would still be ok to put it into 1.2, as its strictly an addition, and we don't touch code that can be replaced / overwritten. the additional css class has no effect until styling rules are added.

but otherwise, i suggest you just create the 1.3 branch from this branch, push that and close the PR. then we have the 1.3 version.

ElectricMaxxx commented 8 years ago

closed due to #286