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.

Sign up for GitHub

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

Jump to bottom

Include YAML frontmatter in the Stork index #398

Merged
merged 18 commits into from
Dec 24, 2022
Merged

Include YAML frontmatter in the Stork index #398

merged 18 commits into from
Dec 24, 2022

Conversation

jfpedroza
Copy link
Contributor

This PR sets the frontmatter_handling setting in Stork to Ignore so the frontmatter is included in the searchable text.

See #397

srid
srid requested changes Dec 11, 2022
View reviewed changes
Copy link
Owner

@srid srid 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 the PR.

Reading your comments here, I'm convinced that we should disable this feature by default and add an option to index.yaml for manually enabling it. We can put the option under: https://github.com/EmaApps/emanote/blob/d2a64234fa327b782fcf7e49097226bea90248fe/default/index.yaml#L131-L133

viz:

emanote:
  stork:
    frontmatter-handling: ignore

src/Emanote/Model/Stork/Index.hs Outdated Show resolved Hide resolved
src/Emanote/Model/Stork/Index.hs Outdated Show resolved Hide resolved
@srid srid added this to the 1.0.2 milestone Dec 11, 2022
jfpedroza
jfpedroza commented Dec 12, 2022
View reviewed changes
Copy link
Contributor Author

@jfpedroza jfpedroza left a comment

Choose a reason for hiding this comment

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

I'm convinced that we should disable this feature by default and add an option to index.yaml for manually enabling it.

I'll explore that part of the code and see how I do.

src/Emanote/Model/Stork/Index.hs Outdated Show resolved Hide resolved
src/Emanote/Model/Stork/Index.hs Outdated Show resolved Hide resolved
@jfpedroza
Copy link
Contributor Author

It now reads the config from index.yaml, defaulting to "omit", the same default from Stork.

Tangential, but if I change the index.yaml, the index isn't rebuilt. I have to write a Haskell or Markdown file for it to be rebuilt. Is that a bug?

srid
srid reviewed Dec 12, 2022
View reviewed changes
Copy link
Owner

@srid srid left a comment

Choose a reason for hiding this comment

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

I left some comments indicating the changes the need to be done on top of this PR. I can do them myself, unless you want to give it a go.

Tangential, but if I change the index.yaml, the index isn't rebuilt. I have to write a Haskell or Markdown file for it to be rebuilt. Is that a bug?

It normally should, IIUC. What browser you are on?

src/Emanote/Model/Stork/Index.hs Outdated
Comment on lines 97 to 98
newtype StorkInput = StorkInput
{ globalInput :: Input
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe, newtype Config and configInput (to keep with the field naming convention).

src/Emanote/Model/Stork/Index.hs Outdated Show resolved Hide resolved
src/Emanote/Model/Stork/Index.hs Outdated Show resolved Hide resolved
@jfpedroza
Copy link
Contributor Author

It normally should, IIUC. What browser you are on?

Firefox.

@jfpedroza
Copy link
Contributor Author

Changed StorkInput to Config and added some docs. I think it's ready.

bors bot added a commit that referenced this pull request Dec 24, 2022
@bors @jfpedroza @srid
Merge #398
b39f130 
398: Include YAML frontmatter in the Stork index r=srid a=jfpedroza

This PR sets the `frontmatter_handling` setting in Stork to `Ignore` so the frontmatter is included in the searchable text.

See #397 


Co-authored-by: Jhon Pedroza <jhon@pedroza.me>
Co-authored-by: Sridhar Ratnakumar <srid@srid.ca>
@srid
Copy link
Owner

srid commented Dec 24, 2022

bors try

bors bot added a commit that referenced this pull request Dec 24, 2022
@bors
Try #398:
7045284
@srid
Copy link
Owner

srid commented Dec 24, 2022

bors r-

@bors
Copy link
Contributor

bors bot commented Dec 24, 2022

Canceled.

@srid
Copy link
Owner

srid commented Dec 24, 2022

bors r-

@srid
Copy link
Owner

srid commented Dec 24, 2022

bors try

@bors
Copy link
Contributor

bors bot commented Dec 24, 2022

try

Already running a review

@bors
Copy link
Contributor

bors bot commented Dec 24, 2022

try

Build succeeded:

srid
srid reviewed Dec 24, 2022
View reviewed changes
src/Emanote/Model/Stork/Index.hs Outdated
Comment on lines 114 to 121
instance FromJSON Handling where
parseJSON = genericParseJSON handlingJSONOptions
where
handlingJSONOptions :: Aeson.Options
handlingJSONOptions =
Aeson.defaultOptions
{ Aeson.constructorTagModifier = toString . T.toLower . T.replace "Handling_" "" . toText
}
Copy link
Owner

Choose a reason for hiding this comment

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

This instance can be simplified using

https://hackage.haskell.org/package/aeson-casing-0.2.0.0/docs/Data-Aeson-Casing.html

Copy link
Owner

Choose a reason for hiding this comment

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

@jfpedroza Wanna take a stab at this?

Otherwise, I'll do the rest of the changes (including this one) tomorrow and merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The package works on fieldLabelModifier, not constructorTagModifier so I can't use the public functions. And even with the internal functions, they don't work on the form Handling_Ignore, they expect camel case.

The only thing useful is applyFirst that allows you to do

Aeson.constructorTagModifier = applyFirst toLower . drop 9

It's not worth to add the package for that function. I added it and changed replace with drop since it avoids the converting to text and back dance.

Copy link
Owner

Choose a reason for hiding this comment

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

Right.

I actually should have suggested https://hackage.haskell.org/package/deriving-aeson because that's exactly what we need here.

cf. https://ghc.gitlab.haskell.org/ghc/doc/users_guide/exts/deriving_via.html

@srid
Copy link
Owner

srid commented Dec 24, 2022

bors try

bors bot added a commit that referenced this pull request Dec 24, 2022
@bors
Try #398:
dc0c0fb
@bors
Copy link
Contributor

bors bot commented Dec 24, 2022

try

Build succeeded:

@srid
Copy link
Owner

srid commented Dec 24, 2022

bors try

bors bot added a commit that referenced this pull request Dec 24, 2022
@bors
Try #398:
7cf6b2d
@bors
Copy link
Contributor

bors bot commented Dec 24, 2022

try

Build succeeded:

@srid srid merged commit eb22a4b into srid:master Dec 24, 2022
@jfpedroza jfpedroza deleted the jp/stork-frontmatter branch December 24, 2022 21:04
shivaraj-bh pushed a commit to shivaraj-bh/emanote that referenced this pull request Dec 18, 2023
@jfpedroza
Add option to include YAML frontmatter in the Stork index (srid#398)
16b21fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srid srid srid requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.2
Development

Successfully merging this pull request may close these issues.
2 participants
@jfpedroza @srid