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

fix(FilePicker): path can now start with a dot #544

Conversation

TungstnBallon
Copy link
Contributor

paths inside FilePicker can now start with a dot

block DataPicker oftype FilePicker {
	path: "./data.csv";
}

closes #381

@TungstnBallon TungstnBallon linked an issue Apr 12, 2024 that may be closed by this pull request
@TungstnBallon TungstnBallon self-assigned this Apr 12, 2024
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

If possible, a great practice when fixing bugs is:

  • Create a unit test that fails with the current bug (here, create a test for the FilePicker blocktype that fails with a leading dot path)
  • Fix the bug

This workflow ensures that the first time a bug is reported it is the last time a human encounters it.

In this case, I think we have unit tests for block types so I'd ask you to add one for this situation. That can be in a separate PR though, after merging this one. You can use "references #381" in the description of that PR to make sure they are nicely linked

@TungstnBallon TungstnBallon force-pushed the 381-bug-file-picker-should-not-crash-for-paths-starting-with-a-dot branch from 1b897b5 to 0a1f33b Compare April 12, 2024 12:17
@rhazn
Copy link
Contributor

rhazn commented Apr 12, 2024

(In case you'd like me to review this again, please re-request a review on the top right @TungstnBallon :)).

@georg-schwarz
Copy link
Member

georg-schwarz commented Apr 15, 2024

Hey @TungstnBallon,

Sorry that this happened the second time now, but there has been a refactoring again 😅
#545 renamed all occurrences of "blocktype" to "blockType", it probably affects some of your imports.
#546 enforces the use of type imports where possible; the auto-fix feature of eslint should be pretty helpful here.

Let me know if I can help updating the branch ;-)

@TungstnBallon TungstnBallon force-pushed the 381-bug-file-picker-should-not-crash-for-paths-starting-with-a-dot branch from 0a1f33b to 2296b1b Compare April 15, 2024 13:37
@TungstnBallon
Copy link
Contributor Author

is there an advantage to putting the tests in a separate pull request? @rhazn

Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

No advantage to do it in a separate PR in general (in fact, changes and their tests should be in one PR). But sometimes it is better to get a smaller PR merged as soon as possible and move related work to future PRs. For example to avoid what happened here with Georg making changes that conflicted with yours in the meantime.

I wasn't sure if you'd need more time to figure out how testing in Jayvee works, but it looks like it went fast so great job 👍.

You can merge this, I'd squash and merge it so it becomes one consistent commit though.

@TungstnBallon TungstnBallon merged commit bc2cf21 into main Apr 15, 2024
3 checks passed
@TungstnBallon TungstnBallon deleted the 381-bug-file-picker-should-not-crash-for-paths-starting-with-a-dot branch April 15, 2024 13:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] File picker should not crash for paths starting with a dot
3 participants