[Added] Now a filebrowser is accessible and attachements can be linked. #54

mbu93 · 2017-05-06T16:33:31Z

The wiki now automatically creates links to the files in the file-folder of the generated site. If there is no file-folder found it will initially create one and add the neccessary paths(same as the html paths). To add a link files can simply be dropped. In order to do so I extended the Server Responses for a few filetypes.

coveralls · 2017-05-06T16:48:11Z

Coverage decreased (-9.6%) to 31.847% when pulling 2cacc34 on mbu93:master into af0ab2e on rust-leipzig:master.

saschagrunert

Thank you very much for the contribution. We need some rework to this pull request.

saschagrunert · 2017-05-15T17:26:35Z

src/lib.rs

        /// Gather new content
        let md_path = PathBuf::from(&directory).join("**").join("*.md");
-        if !Path::new(&directory).is_dir() {
+        if !Path::new(&directory).is_dir () {


Please revert this change :)

saschagrunert · 2017-05-15T17:27:48Z

src/lib.rs

+            info!("Creating directory for file input: {}.", file_directory);
+            fs::create_dir(file_directory);
+        }
+        for path in self.paths.clone(){


This clone can be avoided by iterating over the references: for path in &self.paths

saschagrunert · 2017-05-15T17:28:44Z

src/lib.rs

+            let mut count = 0;
+
+            //create the path for the html site or just find it
+            let page_files_path = format!("{}/{}", file_directory, path.to_str().unwrap()).replace(".md","");


I recommend to use the join based approach here.

saschagrunert · 2017-05-15T17:29:49Z

src/lib.rs

+            //create the path for the html site or just find it
+            let page_files_path = format!("{}/{}", file_directory, path.to_str().unwrap()).replace(".md","");
+            if !Path::new(page_files_path.as_str()).exists(){
+                info!("Creating directory for {}'s files: {}.", path.to_str().unwrap(), page_files_path.as_str());


The unwraps can lead into panics, please avoid these by using the display method.

saschagrunert · 2017-05-15T17:30:15Z

src/lib.rs

+            //add the path
+            let file_path = Path::new(page_files_path.as_str());
+            //read all the files
+            let files = file_path.read_dir().unwrap();


Please add error handling here as well.

saschagrunert · 2017-05-15T17:32:34Z

src/lib.rs

    pub fn serve(&self, output_directory: &str) -> Result<()> {
        // Create a default listening address
-        let addr = "localhost:5000";
+        let addr = "localhost:3000";


I recommend to use a higher ranged port here. The address should be a static string somewhere in the lib, since we have no configuration options.

saschagrunert · 2017-05-15T17:33:39Z

src/lib.rs

        let output_directory_string = output_directory.to_owned();

+        //maybe this shoul be stored in a separated file?
+        let pdf_mime: Mime = "application/pdf".parse().unwrap();


Please avoid "live" unwrapping, this can be done before main in a static string or by using the lazy static crate.

saschagrunert · 2017-05-15T17:34:11Z

src/lib.rs

                if !path.exists() {
-                    return Ok(Response::with(status::InternalServerError));
+                    return Ok(Response::with((ContentType::html().0,
+                    status::NotFound, html_err)));


Wrong indent my dear 🐻

saschagrunert · 2017-05-15T17:35:06Z

src/lib.rs

                }
-
-                let mut f = match File::open(path) {
+                let mut f = match File::open(path.clone()) {


I think you can also open a reference here to avoid cloning.

saschagrunert · 2017-05-15T17:35:31Z

src/lib.rs

+                                status::NotFound, html_err))),
+                    };
+
+                match path.clone().to_str(){


I think we can avoid this clone too.

Drogglbecher

All in all there are many unwraps in waiting to be purged ;)

Drogglbecher · 2017-05-15T18:41:06Z

src/lib.rs

+                    "\n<a href='http://localhost:3000/files/{}/{}'>{}</a><br>\n",
+                    path.to_str().unwrap().replace(".md", ""),
+                    current_entry.file_name().into_string().unwrap().replace("\n","."),
+                    current_entry.file_name().into_string().unwrap()


I think you can try to replace these 3 unwraps above with ok_or_else

Drogglbecher · 2017-05-15T18:43:14Z

src/lib.rs

+
+                let mut html_file = OpenOptions::new().read(true).write(true).create(true).
+                    open(format!("output/{}", path.to_str().unwrap().
+                    replace(".md", ".html"))).unwrap();


These unwraps can be handled too

Drogglbecher · 2017-05-15T18:44:26Z

src/lib.rs

+            }
+            //check if there are no files attached
+            if count == 0 {
+                info!("No files found so far for {}. Simply add your files to the folders in 'files'.", path.to_str().unwrap())


For better reading some line breaks would be good. In l. 186 too.

Drogglbecher · 2017-05-15T18:57:56Z

src/lib.rs


        Ok(())
    }
+    /// read all the files from current paths


This description not really describes what this function actually does (and to be honest, i have no idea too)

Drogglbecher · 2017-05-15T19:00:35Z

src/cli.yml

+      help: Enable integrated HTTP server to serve contents from output directory.
      long: www
      short: w
+  - file_directory:


Since input and output directory we have now additionally a file directory and as a user I would have no idea wherefore this is and the description is not really helpful.

…mited. Better doc comments and minor fixes.

saschagrunert

Looks good now, what do you think @Drogglbecher

Drogglbecher · 2017-06-14T11:18:19Z

Yes, thanks. Please merge in the current master and resolve the failing check, after that I will have a look, it's a little bit hard to read it this way. There are some "coding style issues" too but we can resolve them later ;) @mbu93

Conflicts: Cargo.lock Cargo.toml src/lib.rs src/main.rs

Drogglbecher

Please do cargo update, I think this should resolve the problems with nightly build ;)

After these changes the rest looks ok so far. Thanks ;)

Drogglbecher · 2017-06-25T14:29:24Z

src/lib.rs

+            for entry in files {
+                let current_entry = match entry {
+                    Ok(entry) => entry,
+                    _ => panic!("Couldn't read the path!"),


Better return an error with a meaningful description here.

Drogglbecher · 2017-06-25T14:31:18Z

src/lib.rs

+
+            //create the path for the html site or just find it
+            let page_files_path = file_directory.join(&path.path).to_str().ok_or_else(||
+                                  "Couldn't create Path to html file!")?.replace(".md","");


This error message is a bit weird. It would be also better to break the lines here for better reading.

file_directory.join(&path.path) .to_str() .ok_or_else(|| "Couldn't create Path to html file!")? .replace(".md","");

Drogglbecher · 2017-06-27T10:11:25Z

src/lib.rs

+                                  "Couldn't create Path to html file!")?.replace(".md","");
+
+            if !PathBuf::from(page_files_path.as_str()).exists(){
+                info!("Creating directory for {}'s files: {}.", path.path.to_str().ok_or_else(|| "Path not found!")?,


Would be good when specifying which path can't be found. For this we should rework the error messages for the whole project, there are much things in like this ^^

Drogglbecher · 2017-06-27T10:12:47Z

src/lib.rs

+            let files = file_path.read_dir()?;
+
+            //now attach the files to the html sites folder
+            for entry in files {


Here you can get rid of the extra var but write
for entry in file_path.read_dir()? { ...

Drogglbecher · 2017-06-27T10:15:12Z

src/lib.rs

+                html_file.read_to_string(&mut buffer);
+                buffer += &link.as_str();
+                info!("Creating link to {:?} for {:?}", current_entry.file_name(), path.path.to_str()
+                      .ok_or_else(|| "Entry is corrupted!")?.replace(".md", ".html"));


info!("Creating link to {:?} for {:?}", current_entry.file_name(), path.path.to_str() .ok_or_else(|| "Entry is corrupted!")? .replace(".md", ".html"));

Drogglbecher · 2017-06-27T10:18:58Z

src/lib.rs

+
+                        else {Ok(Response::with((status::Ok, f)))}
+                    },
+                    _ => panic!("Invalid Path."),


Here also return an Error.

[Added] Now a filebrowser is accessible and attachements can be linked.

2cacc34

mbu93 requested review from Drogglbecher and saschagrunert May 6, 2017 16:33

saschagrunert requested changes May 15, 2017

View reviewed changes

Drogglbecher requested changes May 15, 2017

View reviewed changes

Display and join method is implemented now. Clones and unwraps are om…

47654a0

…mited. Better doc comments and minor fixes.

saschagrunert approved these changes Jun 13, 2017

View reviewed changes

MfLabrepo@gmail.com added 2 commits June 17, 2017 01:37

[Improved] Did some coding style changes.

481952c

Merge remote-tracking branch 'upstream/master'

30afb53

Conflicts: Cargo.lock Cargo.toml src/lib.rs src/main.rs

Drogglbecher requested changes Jun 27, 2017

View reviewed changes

Drogglbecher merged commit b67e14b into rust-leipzig:master Jun 29, 2017

[Added] Now a filebrowser is accessible and attachements can be linked. #54

[Added] Now a filebrowser is accessible and attachements can be linked. #54

Uh oh!

Conversation

mbu93 commented May 6, 2017

Uh oh!

coveralls commented May 6, 2017

Uh oh!

saschagrunert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Drogglbecher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschagrunert left a comment

Choose a reason for hiding this comment

Uh oh!

Drogglbecher commented Jun 14, 2017

Uh oh!

Drogglbecher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants