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

Updates for the next Doctum version #94

Merged
merged 5 commits into from
Jan 4, 2021
Merged

Updates for the next Doctum version #94

merged 5 commits into from
Jan 4, 2021

Conversation

williamdes
Copy link
Contributor

@williamdes williamdes commented Dec 17, 2020

Fixes: #59
Closes: #93

@williamdes
Copy link
Contributor Author

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

@williamdes williamdes marked this pull request as ready for review December 20, 2020 14:21
@andrewandante
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

andrewandante commented Dec 20, 2020

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
Copy link
Contributor Author

williamdes commented Dec 20, 2020

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 😆

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

@andrewandante
Copy link
Contributor

Looks good to me and works locally 👍

@williamdes williamdes mentioned this pull request Dec 21, 2020
@chillu
Copy link
Member

chillu commented Jan 4, 2021

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
Copy link
Contributor Author

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: #95 and will have effect after this one also is
Just pushing another commit to upgrade the Doctum version

@williamdes
Copy link
Contributor Author

Rebased and just added 822c2b3

@andrewandante
Copy link
Contributor

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

@andrewandante andrewandante merged commit 8d2b94f into silverstripe:master Jan 4, 2021
@andrewandante
Copy link
Contributor

Amazing work, thank you so much!

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.

Link to Github source code
3 participants