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

[#40, #54] Configuration file with computed fields and include/exclude file options #84

Closed
wants to merge 7 commits into from

Conversation

mohamedsalem401
Copy link
Contributor

@mohamedsalem401 mohamedsalem401 commented Dec 5, 2023

Closes #40
Closes #54

  • Integrate a configuration file options.
  • Implement computed fields.
  • Provide the ability to configure include and exclude file patterns similar to the approach used in Next.js/TypeScript.

Copy link

changeset-bot bot commented Dec 5, 2023

⚠️ No Changeset found

Latest commit: b15bf09

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mohamedsalem401 mohamedsalem401 changed the title Config [#40, #54] Configuration file with computed fields and include/exclude file options Dec 5, 2023
@rufuspollock
Copy link
Member

@mohamedsalem401 does this reflect the comments in the PR yesterday re the definition in the README PR of this feature?

Copy link
Member

@rufuspollock rufuspollock left a comment

Choose a reason for hiding this comment

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

I can already see at least one issue is that the system should continue to work without a config folder.

In general, i'd strongly recommend (as discussed last week) that we get an example working without the config folder stuff but just with passing config to the relevant class or function e.g. buildMarkdownDB or whatever it is called.

once that API is well shaped adding config in a file is relatively straightforward ...

src/bin/index.js Outdated Show resolved Hide resolved
// TODO get these from markdowndb.config.js or something
const dbPath = "markdown.db";
const ignorePatterns = [/Excalidraw/, /\.obsidian/, /DS_Store/];
const [contentPath] = process.argv.slice(2);
const [contentPath, configFilePath] = process.argv.slice(2);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you don't have a config file - this stuff should work with defaults ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rufuspollock
OK, but what should be defaulted?

  1. Currently, it defaults to searching for markdowndb.config.js if no config path is passed.

  2. If markdowndb.config.js is missing, it defaults to - no custom fields - and includes all files.

  3. If a config path is passed but doesn't exist, then it raises an error: Error loading configuration file from....

@rufuspollock
Copy link
Member

@mohamedsalem401 another general comment - i'd have kept the include/exclude feature for a separate PR. It makes the review easier and faster ...

@mohamedsalem401
Copy link
Contributor Author

I will close this for now

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.

Computed metadata fields Config file with basic content include/exclude options
2 participants