Skip to content

feat: add title in orgparse object in title method #23

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

Closed
wants to merge 1 commit into from

Conversation

olopost
Copy link
Contributor

@olopost olopost commented Sep 18, 2020

add title method in org parse object in order to get title options

this point is often standard in org mode file.

thanks for accept the PR.

Best regards

Copy link
Owner

@karlicoss karlicoss left a comment

Choose a reason for hiding this comment

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

Thanks, appreciate! But I'm a bit reluctant to pollute attributes of OrgNode with specific optional org properties.

IMO a cleaner approach would be to have a helper method that allows for easier file properties access (agree that node._special_comments... is pretty awkward).

For example, similarly to get_property method, OrgRootNode could have a get_file_property method? And a get_only_file_property method that returns single value, not a list.
Then your usecase would look like title = root.get_only_file_property('TITLE'). What do you think?

@olopost
Copy link
Contributor Author

olopost commented Sep 19, 2020

Hi just push the update for get_file_property and get_only_file_property

I also change the parse_comment parser in order to ignore space at line beginning.

thanks for your feedback.

Best regards

karlicoss added a commit that referenced this pull request Nov 2, 2020
karlicoss added a commit that referenced this pull request Nov 2, 2020
@karlicoss
Copy link
Owner

karlicoss commented Nov 2, 2020

Hi @olopost sorry your PR has completely slipped off my mind.
On second thought, multivalued properties are pretty rare, so perhaps makes sense to make single valued method name shorter.
I've done the change + some other minor edits (just so we merge it quicker), do you think it looks OK? #28 (I've kept your original commit so you have the attribution!)
Let me know and I'll merge it!

karlicoss added a commit that referenced this pull request Nov 5, 2020
@karlicoss
Copy link
Owner

Merged, but let me know if you have some further comments!

@karlicoss karlicoss closed this Nov 5, 2020
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