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

Added support for optional file include titles #70

Closed
wants to merge 1 commit into from

Conversation

aaslun
Copy link
Contributor

@aaslun aaslun commented Nov 12, 2014

No description provided.

@victorjonsson
Copy link
Owner

Hate to be a downer but I'm having problems with this solution for two reasons:

  • It's implicit. The function arlima_file_args is meant to define the arguments that should be used in the file. Saying that one of these arguments will also be used as a file label in the list manager is implicit.
  • It only works because of the logic that makes the new solution (the array declaration) backwards compatible. Once we decide not to support the old way of declaring file args this will stop working, or we will have to take it in consideration when removing the old code.

We could merge it simply to support the feature and keep the code until this implementation completely changes in accordance to #63 ... or you could think out a different way to solve this. Maybe look at pages/main.php and apply a filter to the output of the file name...

@aaslun
Copy link
Contributor Author

aaslun commented Nov 12, 2014

It's OK, I clearly missed the bigger picture here. Thanks for the constructive comments, I'll take a look at what can be done through applied filters instead.

@aaslun aaslun closed this Nov 12, 2014
@aaslun aaslun deleted the file-include-title branch November 12, 2014 14:56
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