Display all exemplar files #1908
Conversation
app/javascript/components/types.ts
Outdated
| @@ -655,3 +655,8 @@ export type Notification = { | |||
| } | |||
|
|
|||
| type NotificationImageType = 'icon' | 'avatar' | |||
|
|
|||
| export type GitFile = { | |||
There was a problem hiding this comment.
There's not really anything specific to Git for this file. Maybe call this FileWithContents or NamedFile or something else?
There was a problem hiding this comment.
Ah. Good point. For this, I will be very specific and call it MentoringSessionExemplarFile for now. We can then generalize when we have more examples.
| @@ -31,7 +31,7 @@ def to_s | |||
| anonymous_mode: discussion&.anonymous_mode? | |||
| ), | |||
| mentor_solution: mentor_solution, | |||
| exemplar_solution: exercise.exemplar_files.values.first, | |||
| exemplar_files: SerializeGitFiles.(exercise.exemplar_files), | |||
There was a problem hiding this comment.
@kntsoriano I've inlined the serializing here, as it needs custom rules for this situation (stripping the .meta)
| {exemplarFiles.map((file) => { | ||
| return ( | ||
| <div key={file.filename} className="exemplar-files"> | ||
| {exemplarFiles.length > 1 ? ( |
There was a problem hiding this comment.
@kntsoriano I added this so that the filename only shows if there are multiple. That's broken a couple of tests. Could you get them green pls.
| @@ -0,0 +1,23 @@ | |||
| module ReactComponents | |||
There was a problem hiding this comment.
I didn't want to add this to serializers/ because it's been getting harder for me to navigate that directory. I think we ought to have some form of object that is local to the react component it is for. I'll have a play with this approach and see where it takes us.
There was a problem hiding this comment.
Yep. Serializers should only be used for things that use the API. Other things should be inlined like you've done here.
There was a problem hiding this comment.
@kntsoriano I've inlined this into the React component. We don't need extra files for every helper class that's used - we can add things into the one class instead. This is how Rails organises everything internally and it means you don't need to jump around a code base to understand what's going on, which I much prefer.
There was a problem hiding this comment.
My reason with breaking this up was to be able to test only what we've changed. It's getting frustrating that when we change one thing here, we need to fix three other tests. I think this is a symptom of not testing things in the right level.
There was a problem hiding this comment.
Yeah - totally agree with this too. We should only need to change 1 test if this changes. See #1975
| @@ -0,0 +1,25 @@ | |||
| require_relative "../../../../../app/helpers/react_components/mentoring/session/exemplar_file_list" | |||
There was a problem hiding this comment.
I'm testing without requiring Rails. This will make our test suite faster.
There was a problem hiding this comment.
I'm not sure you actually are. If you don't have Rails then ActiveSupport::TestCase below won't exist so it'll fail. I think you're just not referencing test_helper.rb here, which means if you run this file by itself, it'll be fast, but when you run the whole suite, the test_helper will be required for any file, so the whole thing will render normally.
I might be wrong though and it might be faster, but I can't see why or how it would be.
There was a problem hiding this comment.
So changing this back to require test_helper and then running results in this:
So it's already very fast when you just run this as one file. All the slowdown really is when we start to do stuff with databases etc.
There was a problem hiding this comment.
@kntsoriano I've reverted this for now. I'm not against doing things like this, but its important we do them systematically across everything rather than just as a one-off variation. So let's work out what we can do to speed everything up and then implement everywhere :)
There was a problem hiding this comment.
It runs a lot faster without requiring the test helper:
I know that we want to be systematic about things, but having really slow tests and fixing tests that break when we don't want them really makes it frustrating from day to day.
There was a problem hiding this comment.
I know that we want to be systematic about things, but having really slow tests and fixing tests that break when we don't want them really makes it frustrating from day to day.
Totally agree with this. But 0.005s is pretty fast already. Our whole test suite would run in 2s if all the tests were that fast. I don't think this is where the speed improvements are going to come from. There's many tests that take 5s to run, which are 1000x slower than this, which is where I think we need to find solutions.
There was a problem hiding this comment.
The difference is though is when you TDD and run the tests constantly. This is where the difference lies in getting instant feedback versus waiting another multiple of a second. I find that I get to focus easier and I get work done a lot faster.
I could just then do this setup locally and when I'm going to commit the changes, I'm going to just require test helper.
* Display all exemplar files * Only show header if there's multiple files * Fix failing tests * Rename GitFile to MentoringSessionExemplarFile * Use test_helper * Inline exemplar files list Co-authored-by: Jeremy Walker <jez.walker@gmail.com>
Closes exercism/exercism#5871.
Screenshot
It probably needs styling.