search

silverstripe / api.silverstripe.org

API documentation for the Silverstripe Framework
BSD 3-Clause "New" or "Revised" License
3 stars 14 forks source link

Updates for the next Doctum version #94

Closed williamdes closed 3 years ago

williamdes commented 3 years ago

Fixes: https://github.com/silverstripe/api.silverstripe.org/issues/59 Closes: https://github.com/silverstripe/api.silverstripe.org/pull/93

williamdes commented 3 years ago

Thank you @andrewandante ! I added a unit test to prevent regressions and be sure the code works for ever ^^ And I added you as a co-author

andrewandante commented 3 years ago

Test is an excellent addition, thanks!

Had an issue building locally, I've fixed it by adding "3": null to the versionmap for starts-at-three - graphql was throwing me an error otherwise.

williamdes commented 3 years ago

Test is an excellent addition, thanks!

Had an issue building locally, I've fixed it by adding "3": null to the versionmap for starts-at-three - graphql was throwing me an error otherwise.

Ha, is it a hack or a bug fix? Could you send me the patch?

andrewandante commented 3 years ago

I think it's a bugfix - there was no graphql module associated with SS3, but it's trying to figure one out. This is the change to conf/doctum.json:

"versionmaps": {
        "starts-at-one": {
            "master": "master",
            "4": "1",
            "3": null
        },
        "starts-at-three": {
            "master": "master",
+           "4": "3",
+           "3": null
        }
    }

You can see it matches the pattern from the starts-at-one block so I think it makes sense.

Would also be caught by checking isset() on the versionmap:

$projectVersion = isset($versionMaps[$packageConfig['versionmap']][(string) $projectVersion])
  ? $versionMaps[$packageConfig['versionmap']][(string) $projectVersion]
  : $projectVersion;

Probably both changes are good 😄

williamdes commented 3 years ago

Good catch, I added 2 test cases and used the ?? operator for a clean fix just in case. All covered by tests of course ;p

Closing back my workstation to try to go to sleep :laughing:

Maybe Travis will do his job before I wake up, who knows :laughing:

andrewandante commented 3 years ago

Looks good to me and works locally 👍

chillu commented 3 years ago

Phew, that's a lot of changes required for a minor release! But I guess we're "power users" in this aspect (file path mapping). Thanks for keeping on improving this Willian, and happy 2021 to you! I haven't gone through the details on this PR, but if Andrew approves I'm happy with it as well.

williamdes commented 3 years ago

Phew, that's a lot of changes required for a minor release! But I guess we're "power users" in this aspect (file path mapping). Thanks for keeping on improving this Willian, and happy 2021 to you! I haven't gone through the details on this PR, but if Andrew approves I'm happy with it as well.

Ha I made a lot of changes for you upstream also you know ;) This one can be merged: https://github.com/silverstripe/api.silverstripe.org/pull/95 and will have effect after this one also is Just pushing another commit to upgrade the Doctum version

williamdes commented 3 years ago

Rebased and just added 822c2b33551611a950057ad903f1c4c22fe42d90

andrewandante commented 3 years ago

Amazing, I was just doing that myself - I've been granted write access so if you are now happy, I'm happy too 👍

andrewandante commented 3 years ago

Amazing work, thank you so much!