-
Notifications
You must be signed in to change notification settings - Fork 21
allow additional image extensions #160
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
base: devel
Are you sure you want to change the base?
Conversation
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.
please allow the maintainer to perform the version bumps at their discretion
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.
I have "Allow edits by maintainers" checked on the PR menu, is there something else that I need to change to allow maintainers to version bump?
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.
I meant to say that I usually leave the version bumping up to the maintainer so to avoid conflicts with the main branch. That means that I wouldn't commit any changes to the version in the DESCRIPTION file. You may also consider reverting the change to the DESCRIPTION and using git commit --amend and then force pushing to your branch git push -f aghazi addl_img_exts where aghazi is the name of the remote for your fork.
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.
Ah okay. I saw a version bump in a different PR that I was imitating so I thought that was required. I'll try the fix you suggest.
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.
I made the change you suggested 👍
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.
Looks good!
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.
It may be better to use tools::file_ext() and tolower() to check the extension with vector of allowed extensions.
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.
Consider doing something like:
## [... outside the function]
.ALLOWED_FORMATS <- c("png", "jpg", "jpeg", "tif", "tiff")
## [... inside the function]
ext <- tools::file_ext(x) |> tolower()
ext %in% .ALLOWED_FORMATSThere 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.
Okay, I also made this change as well 👍
66970df to
849a6df
Compare
…zed variants. Amended earlier changes that directly bumped the version number. Also changed the pattern detection to use tolower() as suggested.
849a6df to
3f972d4
Compare
LiNk-NY
left a comment
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.
Great, thanks!
As first brought up here #159 , this PR makes a small change to the pattern used in the
.path_validity()function to allows additional image file extensions and their capitalized variants. It increases the "long lines" note fromBiocCheck::BiocCheck()from 25 to 26 but doesn't affect the check or test results otherwise.I tried to follow the contributor guidelines here as best as I could, please let me know if you'd like something changed.