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

feat: convert mbtiles to a styled map package file #35

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

gmaclennan
Copy link
Member

Helpful for when only mbtiles are available as a data source.

@gmaclennan gmaclennan self-assigned this Nov 11, 2024
@gmaclennan gmaclennan requested a review from EvanHahn November 18, 2024 15:41
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM other than a few nitpicks.

const style = await smp.getStyle('')
const sourceMetadata = Object.values(style.sources)[0]
assert.equal(sourceMetadata.type, 'raster')
for (const { x, y, z, data } of mbtiles) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth verifying that mbtiles is nonempty?

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 6b75988

* @param {Record<string, string | number>} variables - An object where the keys correspond to variable names and values correspond to the replacement values.
* @returns {string} The string with the variables replaced by their corresponding values.
*/
export function replaceVariables(template, variables) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be exported?

Suggested change
export function replaceVariables(template, variables) {
function replaceVariables(template, variables) {

Copy link
Member Author

Choose a reason for hiding this comment

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

done in 6b75988

Comment on lines +64 to +66
const returnValue = outputPath
? pipelinePromise(writer.outputStream, fs.createWriteStream(outputPath))
: writer.outputStream
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly about this, but it feels a little cleaner to have this always take an output stream rather than a path. The command could do this transformation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, however I don't think worth the effort at this stage (would also require updating tests and having a way to test the CLI)

@gmaclennan gmaclennan merged commit 18afee4 into main Nov 18, 2024
9 checks passed
@gmaclennan gmaclennan deleted the feat/from-mbtiles branch November 18, 2024 17:34
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.

2 participants