-
Notifications
You must be signed in to change notification settings - Fork 638
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
feat: RepositoryProvider.revision now public property #5500
Conversation
Signed-off-by: Nathan Johnson <[email protected]>
✅ Deploy Preview for nextflow-docs-staging canceled.
|
def 'should have public revision property' () { | ||
given: | ||
def provider = Spy(RepositoryProvider) | ||
when: | ||
provider.revision = 'branch_or_tag' | ||
then: | ||
provider.revision == 'branch_or_tag' | ||
provider.hasProperty('revision') ? true : false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a unit test for this? I don't think the test will actually test anything because Spock can access protected members on a spy anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property getter (i.e. provider.revision == 'branch_or_tag'
), definitely raises a MissingMethodException exception if the field is protected. Although tbh I wasn't expecting that as the test is in the same package, so should have access. The hasProperty
test was my attempt to assert that is is actually property and hence public.
This is however moot given Paolo's comments below which I will address. And this test will go away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option here would be to have test in another package to assert the public API. I don't currently see this pattern in the repo though, so will take you guidance on whether this is required of not.
modules/nextflow/src/main/groovy/nextflow/scm/RepositoryProvider.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Nathan Johnson <[email protected]>
Signed-off-by: Nathan Johnson <[email protected]>
Signed-off-by: Nathan Johnson <[email protected]>
Signed-off-by: Nathan Johnson <[email protected]>
Signed-off-by: Nathan Johnson <[email protected]>
One line change to make the RepositoryProvider.revision a public property. The driver for this is to allow clients to better manage repository validation by allowing the RepositoryProvider to be the source of truth for the current revision that is set.
An alternate approach would be to add revision validation within the RepositoryProvider itself, e.g.
setRevision(String revision, boolean validateRevision = false)
Validation could then proceed by verifying the revision is present in getBranches or getTags.