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

Added configurable delimiters support to Config object #413

Conversation

malcolmhumphreys
Copy link
Contributor

Added configurable delimiters support which is a modified version of #381

@mike1158, @Shootfast, @mplec and @lgritz can you take a look also

  • What do you think colorspace name should be returned from this string "somefile-Gamma22-ACEScg.exr”? Currently we return the longest right most colorspace name e.g. Gamma22 not ACEScg. This is what the old code looked like it did but maybe now with delimiters it should just be the first delimited colorspace found (ACEScg in this example).
  • I have set the default delimiters to be [-, _, /, , .] what are your thoughts?
  • Note. To get the old behaviour you just need to add ‘’ to the delimiters list.

-- CHANGES

  • added getNumDelimiters()
  • added getDelimiterByIndex()
  • added addDelimiters()
  • added clearDelimiters()
  • added containsToken()
  • Set default delimiters [-, _, /, , .]
  • Updated parseColorSpaceFromString to use containsToken()
  • Added unit tests for containsToken() and parseColorSpaceFromString()
  • Updated pyglue and jinglue bindings
  • Added missing EnvironmentMode.java file

- added getNumDelimiters()
- added getDelimiterByIndex()
- added addDelimiters()
- added clearDelimiters()
- added containsToken()
Set default delimiters  [-, _, /, \, .]
Updated parseColorSpaceFromString to use containsToken()
Added unit tests for containsToken() and parseColorSpaceFromString()
Updated pyglue and jinglue bindings
Added missing EnvironmentMode.java file
@scoopxyz
Copy link
Contributor

scoopxyz commented Sep 24, 2016

Makes sense to me. There probably aren't too many cases where that was an
issue, but it's definitely better to be open to different file naming
conventions.

devernay added a commit to MrKepzie/openfx-io that referenced this pull request Feb 1, 2017
@scoopxyz scoopxyz added this to the 1.1.0 milestone Jul 19, 2017
@scoopxyz scoopxyz closed this Oct 27, 2017
@scoopxyz scoopxyz reopened this Oct 27, 2017
devernay added a commit to NatronGitHub/openfx-io that referenced this pull request May 2, 2019
@michdolan
Copy link
Collaborator

Echoing my comments from #381 :

Curious if we feel like the new OCIO v2 file rules section solves this adequately. It's a different solution, but since it supports wildcards and regex it is more flexible. @hodoulp @doug-walker do you think it would be useful to support a special ${name} variable or similar in file rules regex strings to imply the placement of a color space name, rather than needing to have a pattern that accounts for every color space naming convention?

@doug-walker
Copy link
Collaborator

doug-walker commented Apr 21, 2021

Michael, the FileRules does already allow a special rule that applies the parseColorSpaceFromString type matching. An optional parameter could be added to that rule that would specify the delimiters to use. I think it would be cleaner and clearer to put it there rather than add a separate "delimiters" attribute in the config. That would also be a lot easier to implement than allowing a ${name} parameter in the other rule types.

This PR should perhaps be closed. So much has changed since it was authored that it is going to have to be reworked in any case. If people are still needing a delimiters feature, it would perhaps be best for them to create a new feature request Issue.

@michdolan michdolan closed this Jun 27, 2021
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.

None yet

5 participants