search

react-native-community / upgrade-helper

⚛️ A web tool to support React Native developers in upgrading their apps.
https://react-native-community.github.io/upgrade-helper
MIT License
3.67k stars 112 forks source link

Use app name replacement only in the file header and not the content #214

Closed lucasbento closed 4 years ago

lucasbento commented 4 years ago

Summary

We have noticed a good amount of bugs in the last few days because of the replaces we do in order to change RnDiffApp into the name that the user chooses in the interface.

This PR changes the approach of doing this from replacing all the diff changes to only doing so for the file header and the copy file path functionality which kills the bugs we have been experiencing with the View File link and download button.

Test Plan

  1. Put in a few version combinations;
  2. Change the app name;
  3. Check that:
    • The file header is correct;
    • The View File goes to the right place;
    • The copy button copies the correct path with the correct app name.

Do the same without changing the app name.

lucasbento commented 4 years ago

@benomatis can you please take a look? let me know your thoughts!

Unfortunately this supersedes the work you have done in #209.

lucasbento commented 4 years ago

Also, it's good to keep in mind that with this PR we will keep showing stuff like com.rndiffapp in the diff as we will be no longer replacing that.

Given the number of bugs we had related to this I would rather proceed with it without this functionality as it's not that useful IMO.

pvinis commented 4 years ago

While we're here, I'll go ahead and close #208. I don't think it's worth it. We already have a notice above the diffs that the name RnDiffApp is a template thing.

benomatis commented 4 years ago

@lucasbento Looks good, nice and simple ;) ... Though what is remaining that would need the app name in it? Isn't it the hunk only? Anything else? If it's the only one, it wouldn't be complicated to just run that one through a (or the same) replace function, no?

lucasbento commented 4 years ago

@benomatis Looks good, nice and simple ;) ... Though what is remaining that would need the app name in it? Isn't it the hunk only? Anything else? If it's the only one, it wouldn't be complicated to just run that one through a (or the same) replace function, no?

Indeed only the hunks, I looked into replacing the name there but I think this could cause issues performance-wise and I would rather not go down that rabbit hole.

lucasbento commented 4 years ago

I'm gonna merge this (as it's current broken in master) but feel free to ask any questions or request any other implementation @benomatis & @pvinis.

Btw thank you for your continuous help around this, @benomatis!