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

Improve path #2137

Merged
merged 11 commits into from
Mar 9, 2022
Merged

Improve path #2137

merged 11 commits into from
Mar 9, 2022

Conversation

waruqi
Copy link
Member

@waruqi waruqi commented Mar 9, 2022

  • Before adding new features and new modules, please go to issues to submit the relevant feature description first.
  • Write good commit messages and use the same coding conventions as the rest of the project.
  • Please commit code to dev branch and we will merge into master branch in feature
  • Ensure your edited codes with four spaces instead of TAB.

  • 增加新特性和新模块之前,请先到issues提交相关特性说明,经过讨论评估确认后,再进行相应的代码提交,避免做无用工作。
  • 编写友好可读的提交信息,并使用与工程代码相同的代码规范,代码请用4个空格字符代替tab缩进。
  • 请提交代码到dev分支,如果通过,我们会在特定时间合并到master分支上。
  • 为了规范化提交日志的格式,commit消息,不要用中文,请用英文描述。

@waruqi waruqi added this to the v2.6.5 milestone Mar 9, 2022
@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

@xq114 这个 patch 我支持上 path.translate("foo/../xxx", {reduce_dot2 = true}) => xxx 的相对路径处理了,只有 {reduce_dot2 = true} 时候才开启

#2111

不过,还改了一些 path.directory/absolute/join 等对无效路径的处理,有可能会额外暴露出一些 path nil 的情况,可以帮忙测试下,如果没啥问题,我就merge了

@waruqi waruqi merged commit 50c63bf into dev Mar 9, 2022
@waruqi waruqi deleted the path branch March 9, 2022 10:59
@xq114
Copy link
Contributor

xq114 commented Mar 9, 2022

@xq114 这个 patch 我支持上 path.translate("foo/../xxx", {reduce_dot2 = true}) => xxx 的相对路径处理了,只有 {reduce_dot2 = true} 时候才开启

#2111

不过,还改了一些 path.directory/absolute/join 等对无效路径的处理,有可能会额外暴露出一些 path nil 的情况,可以帮忙测试下,如果没啥问题,我就merge了

改个名吧,{simplify = true}或者{normalize = true}更好,最好提供一个path.normalize,和c++标准库保持一致,参考c++标准库对normalize的解释 https://en.cppreference.com/w/cpp/filesystem/path

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

@xq114 这个 patch 我支持上 path.translate("foo/../xxx", {reduce_dot2 = true}) => xxx 的相对路径处理了,只有 {reduce_dot2 = true} 时候才开启
#2111
不过,还改了一些 path.directory/absolute/join 等对无效路径的处理,有可能会额外暴露出一些 path nil 的情况,可以帮忙测试下,如果没啥问题,我就merge了

改个名吧,{simplify = true}或者{normalize = true}更好,最好提供一个path.normalize,和c++标准库保持一致,参考c++标准库对normalize的解释 https://en.cppreference.com/w/cpp/filesystem/path

这个目前没好的办法,这块我有想过,主要是 translate 本身就做了部分 normalize 的事情,比如 /// 去重,去尾部 / ,去重 ./././

额外再加个 normalize 参数仅仅只是去掉 ../../,有点乱,功能划分不明确。

如果单开个 normalize 接口,不仅功能部分重叠,而且很多path 已经执行过 translate 再去执行 normailize 等于遍历了两遍,影响性能。。这两在一起只需要遍历一遍就能搞定。

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

而如果把现有 translate 剥离所有 normalize 功能,又会影响现有各种逻辑,影响太大

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

同样如果改成 simplify = true,目前 translate 现有逻辑已经包含部分 simplify 特性,具体划分到什么程度,光看参数不明确,只能用户查文档后 才知道具体做了什么

@xq114
Copy link
Contributor

xq114 commented Mar 9, 2022

额外再加个 normalize 参数仅仅只是去掉 ../../,有点乱,功能划分不明确。

总比reduce_dot2这种命名好,reduce_dot2初看完全看不出什么意思。而且从用户角度我想知道最后的结果是啥,normalize=true可以告诉我弄完之后就是一个normal path,这样写很合理

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

额外再加个 normalize 参数仅仅只是去掉 ../../,有点乱,功能划分不明确。

总比reduce_dot2这种命名好,reduce_dot2初看完全看不出什么意思。而且从用户角度我想知道最后的结果是啥,normalize=true可以告诉我弄完之后就是一个normal path,这样写很合理

关键是现在不设置 normalize = true,其实也做了大部分 normalize的工作,就差一个 ../.. 处理而已

@xq114
Copy link
Contributor

xq114 commented Mar 9, 2022

关键是现在不设置 normalize = true,其实也做了大部分 normalize的工作,就差一个 ../.. 处理而已

是啊,我觉得直接 标记path.translate为deprecated也ok,大部分时候需要的其实是normalize。说回这里,要加参数总得有个名字,reduce_dot2绝对不行,我不能接受,这名字太奇怪了

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

就算废弃了 translate 全改成 normalize,就跟 把 translate 完整走 normalize 特性,不加参数一样,等于默认内部全 normalize 了,影响有点大,有可能很多地方都会被 broken

而且默认全走了 normalize,处理过于频繁,也影响效率

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

目前折中方案,要么就是改成 normalize = true 参数,部分 translate 中影响不大的去重特性去掉,迁移到 normalize = true 中去

@waruqi
Copy link
Member Author

waruqi commented Mar 9, 2022

改了下,translate 目前 sep 去重还得保留着,剩下的 ././ 和 ../../../ 挪到了 {normalize = true} ,另外加了个 path.normalize 的简化接口,等价于 translate {normalize = true}

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