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: convert output to default to the source root #1579

Merged
merged 14 commits into from
Aug 22, 2024
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Aug 14, 2024

Note that it is diverging a bit from the ideation in #838. Because, in the config file, output defaults to dist (it is where the built files go); in the case of the convert command, I don't think we want that, but rather default to the source root, aka the root option (which defaults to src).

If the --output command-line option is passed explicitly, it must be considered relative to the cwd, not relative to the root. But this means we're comfortable with running this command from anywhere in the project, so if it's not passed explicitly, we make sure to use the source root relative to the root. Otherwise you would end up creating a new "src" folder inside your cwd.

(I was surprised in my tests that the cwd is incorrect when calling yarn tsx ../src/bin/observable.ts convert; I ended up testing manually the built script.)

(I'm making this PR against cobus/convert-documentation but it could be merged afterwards.)

closes #838

@Fil Fil requested a review from mbostock August 14, 2024 14:18
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Functionally good but one quibble on the import. 👍

Also I deleted the command-line flags section from the docs because it’s getting too long and was buried and I think it’s more appropriate to use --help to get help for the command-line flags. (I did include an inline mention of --output though.)

Also, can you hold off on merging this today so that we can document convert as it currently exists rather than blocking the docs & convert blog post on a new release of Framework? I think the current behavior is acceptable since you often have to move things around anyway.

src/bin/observable.ts Outdated Show resolved Hide resolved
Co-authored-by: Mike Bostock <mbostock@gmail.com>
Base automatically changed from cobus/convert-documentation to main August 14, 2024 20:08
@mbostock
Copy link
Member

This needs a re-merge from main to review again. Let me see if I can do it…

@mbostock mbostock enabled auto-merge (squash) August 22, 2024 13:40
@mbostock mbostock merged commit 1570bd7 into main Aug 22, 2024
4 checks passed
@mbostock mbostock deleted the fil/convert-output branch August 22, 2024 13:42
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.

convert should read the config, and respect its output option
3 participants