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

File path went from name to dir #154

Merged

Conversation

Simaris
Copy link
Collaborator

@Simaris Simaris commented Jan 21, 2018

Fix #147
above issue brought a design flaw to my attention, that there since the beginning
and causes us to endlessly redo resource.generate_file_name

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fea768d on Simaris:147/set_proper_page.name_attribute into 790ca72 on white-gecko:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fea768d on Simaris:147/set_proper_page.name_attribute into 790ca72 on white-gecko:develop.

@coveralls
Copy link

coveralls commented Jan 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 734295c on Simaris:147/set_proper_page.name_attribute into bd29794 on white-gecko:develop.

@Simaris Simaris requested a review from white-gecko January 21, 2018 20:52
@white-gecko
Copy link
Member

white-gecko commented Jan 23, 2018

$ ls ../jekyll/_site/
C  README.md  Screenshot.png  d  e  feed.xml  html  rdf-data  rdfsites  t

@Simaris
Copy link
Collaborator Author

Simaris commented Jan 26, 2018

I added a fix for the problem above.
Please check if it is fixed

white-gecko
white-gecko previously approved these changes Feb 8, 2018
@white-gecko white-gecko dismissed their stale review February 8, 2018 12:36

Falsly approved

@white-gecko
Copy link
Member

white-gecko commented Feb 8, 2018

Please check this and see how to make the fix of #155 still work.
Also please fix the tests to also cover this behavior.

@Simaris Simaris force-pushed the 147/set_proper_page.name_attribute branch from 8049a6a to 2d413d1 Compare April 6, 2018 22:56
@white-gecko white-gecko added this to the 3.0.1 milestone Apr 9, 2018
@@ -116,6 +116,12 @@ def filename(domain_name, baseurl)
@filename ||= generate_file_name(domain_name, baseurl)
end

def filedir
return @filedir unless @filedir.nil?
generate_file_name(@site.config["url"], @site.config["baseurl"]) #TODO change to RdfHelper.... like page_url
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the TODO

@@ -116,6 +116,12 @@ def filename(domain_name, baseurl)
@filename ||= generate_file_name(domain_name, baseurl)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove domain_name and baseurlparameters from filename method.

@@ -116,6 +116,12 @@ def filename(domain_name, baseurl)
@filename ||= generate_file_name(domain_name, baseurl)
end

def filedir
return @filedir unless @filedir.nil?
generate_file_name(@site.config["url"], @site.config["baseurl"]) #TODO change to RdfHelper.... like page_url
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the TODO line

@white-gecko white-gecko changed the title file path went from name to dir WIP: File path went from name to dir Apr 9, 2018
@Simaris Simaris force-pushed the 147/set_proper_page.name_attribute branch from 2d413d1 to 4e15c97 Compare April 10, 2018 17:47
@Simaris Simaris requested a review from white-gecko April 12, 2018 14:14
@Simaris Simaris changed the title WIP: File path went from name to dir File path went from name to dir Apr 12, 2018
domain_name = domain_name.to_s
baseurl = baseurl.to_s
domain_name = URI::split(Jekyll::JekyllRdf::Helper::RdfHelper::site.config["url"])[2].to_s
baseurl = Jekyll::JekyllRdf::Helper::RdfHelper::site.config["baseurl"].to_s
Copy link
Member

Choose a reason for hiding this comment

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

What does happen, of no baseurl is set in the config? i.e. if there is no config parameter for baseurl at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

then the value returned by config would be Nil. For that reason, we use the .to_s method to cast it to string.
Nil cast to String becomes "", which means it has no further effect.

@Simaris Simaris force-pushed the 147/set_proper_page.name_attribute branch from 0137c87 to 8c194de Compare April 12, 2018 14:46
@Simaris Simaris requested a review from white-gecko April 14, 2018 17:39
Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

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

With this fix the page is not rendered to the usual path.

@Simaris Simaris changed the title File path went from name to dir WiP: File path went from name to dir Apr 17, 2018
@Simaris
Copy link
Collaborator Author

Simaris commented Apr 17, 2018

made the fix but the index.html => .html bug keeps now coming up

also, I still need to test page.name

@Simaris Simaris force-pushed the 147/set_proper_page.name_attribute branch 2 times, most recently from 993c212 to ae22032 Compare April 19, 2018 15:44
the methods page_url, render_path, filename and filedir will now yield correct
results, no matter in what order they are used.
filename param were removed.
@Simaris Simaris force-pushed the 147/set_proper_page.name_attribute branch from ae22032 to 734295c Compare April 19, 2018 16:37
@Simaris Simaris changed the title WiP: File path went from name to dir File path went from name to dir Apr 19, 2018
@white-gecko white-gecko merged commit a32e18f into AKSW:develop May 3, 2018
@Simaris Simaris deleted the 147/set_proper_page.name_attribute branch May 6, 2018 15:27
@white-gecko white-gecko modified the milestones: 3.0.1, 3.0.0 Oct 20, 2018
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.

page.name is not correct for rdf resource
3 participants