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: Support configurable location for creating notes #331

Merged
merged 6 commits into from
May 14, 2021

Conversation

caiych
Copy link
Contributor

@caiych caiych commented Apr 9, 2021

#38

My apologize in advance -- I'm not fluent in TS. Please just let me know if I'm making any dumb mistake.

Thanks.

@svsool
Copy link
Owner

svsool commented Apr 11, 2021

Thanks for the contribution! 💙I'll have a look next week.

Copy link
Owner

@svsool svsool left a comment

Choose a reason for hiding this comment

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

Thanks for your patience.

I tested this feature on MacOS, and it works with short refs, I don't have Windows at the moment to check if it works there as well (tests seem to pass, though).

I also left some ideas and comments for improvements.

src/commands/openDocumentByReference.spec.ts Outdated Show resolved Hide resolved
src/commands/openDocumentByReference.spec.ts Outdated Show resolved Hide resolved
src/commands/openDocumentByReference.spec.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/commands/openDocumentByReference.spec.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
// Apply default folder rule if it's a short ref(i.e. doesn't have an existing dir in ref).
const defaultPath =
paths.length === 1
? getMemoConfigProperty('files.defaultPaths', []).find((rule) =>
Copy link
Owner

Choose a reason for hiding this comment

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

Great to validate that rules config conforms to the expected format when Memo extension is initialized or this property changed, otherwise it can lead to unexpected runtime exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What kind of validation/runtime exception do you have on your mind?

I couldn't think of any meaningful check actually -- not anything that could prevent a runtime error

Copy link
Owner

@svsool svsool Apr 22, 2021

Choose a reason for hiding this comment

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

Maybe validating that rule and folder are strings and defined, because anything can be passed by user.

At least wrapping it into try catch and reporting it using an error popup with some meaningful description might be better to not break links creation.

Copy link
Owner

Choose a reason for hiding this comment

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

It seems that vscode does not provide any schema for array props, so perhaps should be done manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try catch added.

Any suggestion validating the schema? It seems to be just convenient(and idiomatic) to just do type assertion for settings?

src/types.ts Outdated Show resolved Hide resolved
src/commands/openDocumentByReference.spec.ts Outdated Show resolved Hide resolved
src/commands/openDocumentByReference.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@62dda4d). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #331   +/-   ##
=========================================
  Coverage          ?   91.85%           
=========================================
  Files             ?       21           
  Lines             ?      921           
  Branches          ?      211           
=========================================
  Hits              ?      846           
  Misses            ?       74           
  Partials          ?        1           
Impacted Files Coverage Δ
src/utils/utils.ts 100.00% <ø> (ø)
src/commands/openDocumentByReference.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62dda4d...f9a02ad. Read the comment docs.

@caiych caiych requested a review from svsool April 21, 2021 15:49
@svsool svsool changed the title feat: Support configurable root location for creating notes feat: Support configurable location for creating notes Apr 22, 2021
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@svsool svsool left a comment

Choose a reason for hiding this comment

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

I don't have a Windows VM atm to check if it works there as well. Do you have Windows? It would be great to check that this feature works there, especially such paths "folder": "/Notes", sometimes tests are not super reliable way to prove something works with Windows.

src/utils/utils.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@caiych
Copy link
Contributor Author

caiych commented May 5, 2021

Sorry I was busy lately.

I don't have a windows as well. Sorry.

@caiych caiych requested a review from svsool May 5, 2021 14:00
@svsool
Copy link
Owner

svsool commented May 14, 2021

It was busy time for me as well, thanks for considering all comments!

@svsool svsool closed this May 14, 2021
@svsool svsool reopened this May 14, 2021
@svsool svsool merged commit 59081ba into svsool:master May 14, 2021
@caiych caiych deleted the config branch May 15, 2021 03:15
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.

3 participants