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

Implement functions to replace text in one or more files using regular expressions for the search pattern #649

Merged
merged 2 commits into from
Feb 12, 2015

Conversation

ryanewtaylor
Copy link
Contributor

This is my attempt at addressing #622 - Replacements in ReplaceInFiles should access regex patterns.

This implementation requires that the caller specify the encoding for the files that they are processing. I attempted a solution whereby I did not specify the encoding in the ReadAlltext and WriteAlltext methods but I found that .NET was not always guessing the encoding correctly. As a result this process modified the files from their original encoding (ANSI) to UTF-8. As far as I know there isn't a simple way to detect file encoding so in this implementation I put the responsibility on the caller to specify the encoding.

…sprojects#622

The current implementation requires that the caller specify the encoding
for the files that they are processing. Future enhancements may include
auto-detection of the file encoding to make usage a little cleaner.
@forki
Copy link
Member

forki commented Feb 9, 2015

mhm. cool contribution.

I think it should be splitted in a function which handles the encoding and one that covers the replacement.
maybe also create a overload which doesn't use the encoding overload.

@ryanewtaylor
Copy link
Contributor Author

My apologizes. I do not understand the suggestion to split the function to handle the encoding vs. handling the replacement. The built in RegEx.Replace method handles the replacement and I'm not clear how I would further separate this functionality. Could you elaborate more on this?

Regarding your second suggestion. I did attempt to implement an overloaded function to negate the need to always specify the encoding. However, as I am very new to F# I was unable to determine how to make an overloaded function or a function with a default parameter. From what I could gather default parameters are allowed in functions but not when the function is a part of a let binding.

I was also unsure what a good default would. Three options that came to mind would be:

  1. Do not pass any encoding to ReadAllText or WriteAllText. However the side effect is that WriteAllText writes out files as UTF-8 without a BOM if you do not specify an encoding. This has the potential to change the encoding of the original file if it were not UTF-8 so I opted to not do this.
  2. Use Encoding.Default as the default parameter. However, this suffers from the same problem as not specifying the encoding as we may inadvertently change a files encoding.
  3. Attempt to detect the encoding of the file and use that as the default for both ReadAllText and WriteAlltext. From what I have read on detecting encoding, this is the most involved and would be difficult to get right.

Neither option 1 or 2 seemed particularly right. For instance, in my project (a mix of .NET and Flex) we have UTF-8 files mixed with ANSI and both options would cause unwanted encoding changes in the files. Any suggestions on a good default encoding?

Ultimately, I think option 3 would be the right solution. We could have a RegexReplaceInFile that does not take an encoding argument. This method would detect the encoding of the file. Once detected RegexReplaceInFileWithEncoding would be called with the appropriate parameters and the detected encoding. I read up on detecting encoding and it is by no means trivial so I did not attempt this for my first contribution. Perhaps detecting the encoding could be added as an enhancement in the future?

forki added a commit that referenced this pull request Feb 12, 2015
Implement functions to replace text in one or more files using regular expressions for the search pattern
@forki forki merged commit ac38530 into fsprojects:master Feb 12, 2015
@forki
Copy link
Member

forki commented Feb 12, 2015

Thanks for contributing this.

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.

2 participants