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

Rename files in src/view/results to match their components #2716

Merged
merged 4 commits into from
Aug 17, 2023

Conversation

robertbrignull
Copy link
Contributor

We have a bunch of files in src/view/results where the filename does not match the name of the component they contain. We may as well rename them all at once.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team August 16, 2023 09:45
@robertbrignull robertbrignull requested a review from a team as a code owner August 16, 2023 09:45
@robertbrignull
Copy link
Contributor Author

I don't know what's going on with this Graph import.

If I import "./Graph" then it works locally when I run npm run build, and this makes sense to me because that's what the file is called. However my VS Code complains locally and it fails in CI.

If I rename the import to "./graph" then VS Code is happy, but npm run build fails locally. 😕

Is there something special about this file? It's not failing on any of the other imports. I was wondering if this file is imported from somewhere else like a special javascript file, because the graph viewer does do some unusual things, but I can't find anything like that.

@robertbrignull
Copy link
Contributor Author

This is the error I get locally in VS Code:
Screenshot 2023-08-16 at 11 51 00

This was fixed by a restart. This maybe makes sense that VS Code was confused by the renamed file and had to have the typescript server be restarted. This thread helped me work it out.

Unfortunately that doesn't explain why CI is having problems, because everything in that thread is about needing a restart or prompting typescript to see the new files.

I am wondering if maybe we should change the tsconfig "include": ["src/**/*.ts"] to "include": ["src/**/*.ts(x)"], but I'm not confident on that.

@robertbrignull
Copy link
Contributor Author

I worked out the problem is that I didn't actually commit the graph.tsx => Graph.tsx rename, because I'm on OSX so its case-insensitive filesystem didn't see the rename. I worked around this by doing the rename in multiple steps by renaming to a completely different intermediate name. I then squashed the commits and git seems to have kept the rename.

I hope this works and doesn't cause problems for anyone else who views this repo on OSX. I had problems when changing branch because it didn't update the graph file and it kept the wrong case. 😟

We could fully rename the file but I can't think of a good thing to call it. Or just accept we might have some issues when this PR is merged and people pull the changes.

Copy link
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and works locally. I got the same error as you but it went away after restarting VS Code.

Perhaps git mv would have helped with the renaming issues you came across?

@robertbrignull
Copy link
Contributor Author

Perhaps git mv would have helped with the renaming issues you came across?

Good point. git mv probably would have helped. But git knows about the move now so I don't think it'll change how it behaves for other people pulling this code once merged.

@robertbrignull robertbrignull merged commit 497dc31 into main Aug 17, 2023
26 checks passed
@robertbrignull robertbrignull deleted the robertbrignull/data-renames branch August 17, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants